calcite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Hyde <jh...@apache.org>
Subject Re: New Defects reported by Coverity Scan for Apache Calcite
Date Fri, 04 Dec 2015 19:24:56 GMT
I am currently the SPOF (single point of failure) with Bosco, as it turns out, keeping an eye
out in case I miss something. But a single point of failure seems preferable to sending the
reports to the whole dev list NO ONE taking responsibility.

The best strategy, in fact, is for all contributors to take responsibility for quality. (Committers
need to take responsibility for their own work AND the work of the contributor that they are
committing.) Run your own favorite quality checks (your IDE, findbugs) and you’ll find bugs
in your own code and maybe someone else’s. If we all run the same checks we won’t get
as much coverage.

Julian


> On Dec 3, 2015, at 3:57 PM, Josh Elser <elserj@apache.org> wrote:
> 
> Yeah, I have already registered, Bosco.
> 
> I'd rather not become a SPOF though. If we send to the list, the load can be spread (these
kinds of fixes are great intro-tasks for people).
> 
> Don Bosco Durai wrote:
>> Josher
>> 
>> If you are a committer in Calcite, then you can register yourself at https://scan.coverity.com/projects/apache-calcite?tab=overview.
The scans are done twice a week and if there are any issues found, then the email is automatically
sent.
>> 
>> Bosco
>> 
>> 
>> 
>> 
>> 
>> On 12/3/15, 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