calcite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Hyde <jh...@apache.org>
Subject Re: FW: New Defects reported by Coverity Scan for Apache Calcite
Date Thu, 03 Dec 2015 23:32:43 GMT
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