[
https://issues.apache.org/jira/browse/LUCENE-6744?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15030614#comment-15030614
]
Erick Erickson commented on LUCENE-6744:
----------------------------------------
Well, a few things:
> I didn't offer to shepherd it through and commit it, just answered a question
> on how to get the ball rolling.
> There are no tests that unexpectedly fail with the old code and succeed with
> the new code.
> Most importantly, I'm not a fan of changing behavior to conform to abstract
> principle without there being a demonstrable problem. In this case the
> argument is that using instanceof is incorrect and we should switch to
> getClass() because it's "more correct" and "honors the contract". This will
> change _behavior_ and nobody has shown that this current behavior is
> unintended. And rather than change to getClass(), an alternative is to make
> either the equals method in the superclass final (OK, this won't work for
> ValueSource-derived classes) or the entire class that uses instanceof in the
> equals method final. Are either of those more appropriate? This latter is
> true for both Bitset classes for instance.
For example, looking at the history for ConstValueSource.java,
[[email protected]] committed the original code and the equals method used
getClass. Then as part of SOLR-1568 he changed it to use instanceof. I don't
know why, but I'm not about to undo an intentional change on his part without
asking about it.
The two Bitset classes are final classes, and according to a quote in one of
the links mentioned it's OK to use instanceof in the equals method in that
case. Perhaps using getClass wouldn't be appropriate here, but I'd like
[~shaie] to weigh in.
So you see where this is going. I want the authors of the code, or at least
people who've done maintenance on them to weigh in on why instanceof was used
before changing code:
[[email protected]] for
ConstValueSource.java
DoubleConstValueSource.java
ValueSourceRangeFilter.java
[~shaie] for (although these are final classes so it's probably OK but does it
matter?)
FixedBitSet.java
LongBitSet.java
[~gsingers] for
GeohashFunction.java
StringDistanceFunction.java
VectorDistanceFunction.java
I've made too many gratuitous changes that weren't actually fixing something to
go down that road without checking ;)
Now, all that notwithstanding, the patch passes precommit and test so at least
it didn't break any explicit tests that depended on the behavior of instanceof
and I can argue that if instanceof is intentionally used in preference to
getClass then there _should_ be tests that fail if we use getClass....
And don't get me wrong. Assuming the authors of the code are OK, with it I'm
perfectly happy to commit this patch...
> equals methods should compare classes directly, not use instanceof
> ------------------------------------------------------------------
>
> Key: LUCENE-6744
> URL: https://issues.apache.org/jira/browse/LUCENE-6744
> Project: Lucene - Core
> Issue Type: Bug
> Reporter: Hoss Man
> Labels: newdev
> Attachments: LUCENE-6744.patch
>
>
> from a 2015-07-12 email to the dev list from Fuxiang Chen...
> {noformat}
> We have found some inconsistencies in the overriding of the equals() method
> in some files with respect to the conforming to the contract structure
> based on the Java Specification.
> Affected files:
> 1) ConstValueSource.java
> 2) DoubleConstValueSource.java
> 3) FixedBitSet.java
> 4) GeohashFunction.java
> 5) LongBitSet.java
> 6) SpanNearQuery.java
> 7) StringDistanceFunction.java
> 8) ValueSourceRangeFilter.java
> 9) VectorDistanceFunction.java
> The above files all uses instanceof in the overridden equals() method in
> comparing two objects.
> According to the Java Specification, the equals() method must be reflexive,
> symmetric, transitive and consistent. In the case of symmetric, it is
> stated that x.equals(y) should return true if and only if y.equals(x)
> returns true. Using instanceof is asymmetric and is not a valid symmetric
> contract.
> A more preferred way will be to compare the classes instead. i.e. if
> (this.getClass() != o.getClass()).
> However, if compiling the source code using JDK 7 and above, and if
> developers still prefer to use instanceof, you can make use of the static
> methods of Objects such as Objects.equals(this.id, that.id). (Making use of
> the static methods of Objects is currently absent in the methods.) It will
> be easier to override the equals() method and will ensure that the
> overridden equals() method will fulfill the contract rules.
> {noformat}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
