> This makes jdk.javadoc compare javax.lang.model primitives correctly:
> elements with `Object.equals` and type mirrors with `Types.isSameType`.
>
> jdk.javadoc sometimes compares elements using identity comparison (`==`) and
> type mirrors using equality comparison (`equals`). While this is generally
> wrong, we likely get away with it because of the implementation specifics.
> I've heard that javac is very forgiving in the sense that there's no
> difference between `==` and `equals` for elements. In addition to that, I
> noticed that more often than not there's no difference between `equals` and
> `isSameType` for type mirrors in the context of jdk.javadoc.
>
> Since there's no guarantee that a different compiler implementation or the
> future javac itself will be implemented similarly, it's better to abandon
> these bad practices while we are still lucky.
>
> ----
>
> With a change like this, there are two main concerns. The first is nullity.
> When it's not immediately obvious if `a`, `b`, or both can be null, it's not
> clear how to check for equality. To avoid introducing bugs to a code that I
> don't know that much, I use the `java.util.Objects.equals` method, which
> takes care of nullity the same way the existing code does: if any of `a` or
> `b` are null, `Objects.equals(a, b)` behaves exactly as `a == b`, provided
> `equals` is well-defined and returns false for the null argument.
>
> The second concern is identity. If equality does not imply identity, it might
> be important to distinguish between equal elements and identical elements.
> There are a few cases in jdk.javadoc where it's identity that is required;
> for example, consider:
>
> SortedSet<ExecutableElement> members =
> utils.serializationMethods(currentTypeElement);
> for (ExecutableElement member : members) {
> currentMember = member;
> Content methodsContent = methodWriter.getMethodsContentHeader(
> currentMember == members.last());
> ...
>
> Here's the loop computes a boolean indicating the last iteration. If
> `members` contained identical elements, then that check could misjudge the
> last iteration and exit prematurely. Thankfully, in that and the similar
> cases, iteration happens on a (sorted) set, which by definition cannot have
> duplicates, provided the comparator used by the set is consistent with
> `equals`, which is also consistent with `==`. So in this case we can safely
> change `==` to `equals`.
>
> However, when I fixed those iterations to use `equals`, it didn't feel right;
> it felt marginally less brittle than what there was before. I then
> restructured the loops to avoid equality comparison altogether, which had a
> side-benefit of widening abstractions from `SortedSet` to just `Iterable`.
Pavel Rappo has updated the pull request incrementally with two additional
commits since the last revision:
- Update copyright years
- Clean up emptiness conditions
Removes one useless condition; restructures the other, as suggested in
the review. In each case moves the iterator into the loop to reduce
that iterator's scope.
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/12369/files
- new: https://git.openjdk.org/jdk/pull/12369/files/69e98e22..cb56cfe9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12369&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12369&range=00-01
Stats: 24 lines in 4 files changed: 1 ins; 5 del; 18 mod
Patch: https://git.openjdk.org/jdk/pull/12369.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12369/head:pull/12369
PR: https://git.openjdk.org/jdk/pull/12369
