On Wed, 5 Jul 2023 15:56:51 GMT, Hannes Wallnöfer <hann...@openjdk.org> wrote:
> Please review a fix to make the comparator used by the JavaDoc index pages to > be transitive (see JBS issue for a description and example of the problem). > > I fix the bug by creating method `getIndexElementKey(Element)` to extract the > key for comparing elements in `Comparator.makeIndexElementComparator`, and > allowing `IndexBuilder` to reuse that extractor method when comparing element > index items to search tag index items. The rationale for this approach was to > preserve the current order for sorting elements in the index, and to keep the > change simple (considering possible backport to 21). > > In the process I also simplified some parts of the code a bit (simpler logic > to compare names in element comparator, no need for composite comparator for > classes-only index). > > For the test, I added various elements and search tags that trigger the > condition to the existing `TestIndex` page and replaced `checkOutput` with > `checkOrder` to test the order of index page items. I can see that I'm late to the party. FWIW, the change looks good to me. There's probably one additional problem that predates this PR. I see that Comparators.makeIndexElementComparator breaks ties by comparing fully qualified names. Although unlikely to happen in practise, in theory it might break when one has a static nested class and a class in the default package (hence not applicable to JDK) that share FQN. Here's an example I used for one of our tests earlier (you might need to scroll it if reading inline on GitHub): https://github.com/openjdk/jdk/blob/0616648c59215d001211423402c6444ce228f01e/test/langtools/jdk/javadoc/doclet/testThrowsInheritanceMatching/TestExceptionTypeMatching.java#L350-L396 ------------- PR Comment: https://git.openjdk.org/jdk/pull/14776#issuecomment-1623486580
