Mailing-List: contact dev-help@calcite.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@calcite.apache.org
Message-ID: <5660D522.9000005@gmail.com>
Date: Thu, 03 Dec 2015 18:49:54 -0500
From: Josh Elser <josh.elser@gmail.com>
User-Agent: Postbox 3.0.11 (Macintosh/20140602)
MIME-Version: 1.0
To: dev@calcite.apache.org
Subject: Re: FW: New Defects reported by Coverity Scan for Apache Calcite
References: <56609508dcacd_2dd3885338366b5@ss1435.mail>
 <5B7DD02C-0E0F-4007-88D5-7F3D85C0DBDD@apache.org>
 <CAPSgeETZ_-LgaC27ktUYUOj_XGDSHCwEJxTbBFMwYa55qY8pLQ@mail.gmail.com>
 <CAPSgeER4kHj=VhkxWgmeMO65faB5Ue6-C8J1xJby5hyYU=UFCw@mail.gmail.com>
In-Reply-To: 
 <CAPSgeER4kHj=VhkxWgmeMO65faB5Ue6-C8J1xJby5hyYU=UFCw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Alrighty, easy enough.

Julian Hyde wrote:
> Intellij highlights several other bugs in that file: lines 398, 1036,
> 1246, 1326 (possible), 2266. Please fix these also.
>
> On Thu, Dec 3, 2015 at 2:59 PM, Julian Hyde<jhyde@apache.org>  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
>>>>