calcite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Josh Elser <els...@apache.org>
Subject Re: FW: New Defects reported by Coverity Scan for Apache Calcite
Date Thu, 03 Dec 2015 23:55:02 GMT
Yeah, that's my common interaction with Coverity.

IMO, I'd prefer setting up something like findbugs and running that at 
build time (alongside checkstyle) for static analysis. But, that's just 
me not wanting to be tied to a specific IDE.

Julian Hyde wrote:
> We could, I suppose. I receive and read the emails, and previously
> everything Coverity has found has been a false positive, so it's
> probably wasting people's time.
>
> More useful would be for everyone to switch to Intellij and manually
> check every yellow-barred piece of code in each file they are editing.
> (Or enable the equivalent feature in Eclipse.)
>
> Intellij found 5 bugs in that file. The 4 other things it found (two
> spurious "public"s, an exception being used but not thrown, and an
> unnecessary box) are all things I would have fixed too, in the
> interests of making boring code look boring.
>
>
> On Thu, Dec 3, 2015 at 3:21 PM, Josh Elser<elserj@apache.org>  wrote:
>> Alright, will do. Thanks for bringing it up.
>>
>> Do we want to just set up Coverity so that we get these emails directly?
>>
>> Julian Hyde wrote:
>>> Bosco,
>>>
>>> Yes, this is a problem. Thanks for forwarding.
>>>
>>> Josh,
>>>
>>> You made this change yesterday. Can you please fix?
>>>
>>> I see quite a few examples in that file that should be boilerplate. Why
>>> does
>>>
>>>     if (obj == null || !(obj instanceof RpcMetadataResponse)) {
>>>       return false;
>>>     }
>>>
>>> depart from
>>>
>>>     if (!(obj instanceof RpcMetadataResponse)) {
>>>       return false;
>>>     }
>>>
>>> which is sufficient and is used elsewhere in the file? Boring stuff
>>> should look boring.
>>>
>>> In fact the whole method could be simplified. Rather than
>>>
>>> public boolean equals(Object obj) {
>>>     if (this == obj) {
>>>       return true;
>>>     }
>>>
>>>     if (obj == null || !(obj instanceof RpcMetadataResponse)) {
>>>       return false;
>>>     }
>>>
>>>     RpcMetadataResponse other = (RpcMetadataResponse) obj;
>>>     if (serverAddress == null) {
>>>       if (other.serverAddress != null) {
>>>         return false;
>>>       }
>>>     }
>>>     return serverAddress.equals(other.serverAddress);
>>> }
>>>
>>> there is a case to be made that
>>>
>>> public boolean equals(Object obj) {
>>>     return this == obj
>>>       || obj instanceof RpcMetadataResponse
>>>       &&   Objects.equals(((RpcMetadataResponse) obj).serverAddress,
>>> serverAddress);
>>> }
>>>
>>> is more understandable as well as more concise.
>>>
>>> Elsewhere in the file there is -- not written by you -- the following:
>>>
>>>     if (null == connectionId) {
>>>       if (null != other.connectionId) {
>>>         return false;
>>>       }
>>>     } else if (!connectionId.equals(other.connectionId)) {
>>>       return false;
>>>     }
>>>
>>>     return true;
>>>
>>> It works, but not obviously so.
>>>
>>> Julian
>>>
>>>
>>> On Thu, Dec 3, 2015 at 11:21 AM, Bosco Durai<bdurai@hortonworks.com>
>>> wrote:
>>>>> 3195           return serverAddress.equals(other.serverAddress);
>>>> Good catch. I know you are monitoring it. Just wanted double sure about
>>>> it…
>>>>
>>>> Thanks
>>>>
>>>> Bosco
>>>>
>>>>
>>>>
>>>>
>>>> On 12/3/15, 11:16 AM, "scan-admin@coverity.com"<scan-admin@coverity.com>
>>>> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> Please find the latest report on new defect(s) introduced to Apache
>>>>> Calcite found with Coverity Scan.
>>>>>
>>>>> 1 new defect(s) introduced to Apache Calcite found with Coverity Scan.
>>>>>
>>>>>
>>>>> New defect(s) Reported-by: Coverity Scan
>>>>> Showing 1 of 1 defect(s)
>>>>>
>>>>>
>>>>> ** CID 120367:  Null pointer dereferences  (FORWARD_NULL)
>>>>> /avatica/src/main/java/org/apache/calcite/avatica/remote/Service.java:
>>>>> 3195 in
>>>>> org.apache.calcite.avatica.remote.Service$RpcMetadataResponse.equals(java.lang.Object)()
>>>>>
>>>>>
>>>>>
>>>>> ________________________________________________________________________________________________________
>>>>> *** CID 120367:  Null pointer dereferences  (FORWARD_NULL)
>>>>> /avatica/src/main/java/org/apache/calcite/avatica/remote/Service.java:
>>>>> 3195 in
>>>>> org.apache.calcite.avatica.remote.Service$RpcMetadataResponse.equals(java.lang.Object)()
>>>>> 3189           RpcMetadataResponse other = (RpcMetadataResponse) obj;
>>>>> 3190           if (serverAddress == null) {
>>>>> 3191             if (other.serverAddress != null) {
>>>>> 3192               return false;
>>>>> 3193             }
>>>>> 3194           }
>>>>>>>>       CID 120367:  Null pointer dereferences  (FORWARD_NULL)
>>>>>>>>       Calling a method on null object "serverAddress".
>>>>> 3195           return serverAddress.equals(other.serverAddress);
>>>>> 3196         }
>>>>> 3197       }
>>>>> 3198     }
>>>>> 3199
>>>>>
>>>>>
>>>>>
>>>>> ________________________________________________________________________________________________________
>>>>> To view the defects in Coverity Scan visit,
>>>>> https://scan.coverity.com/projects/apache-calcite?tab=overview
>>>>>
>>>>> To manage Coverity Scan email notifications for "bosco@apache.org",
>>>>> click
>>>>> https://scan.coverity.com/subscriptions/edit?email=bosco%40apache.org&token=368c97cfe2c99af46cce4f2657f16fe3
>>>>>

Mime
View raw message