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 22:59:47 GMT
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