On Wed, 1 Feb 2023 19:09:21 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> 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.
Marked as reviewed by jjg (Reviewer).
-------------
PR: https://git.openjdk.org/jdk/pull/12369
