On Wed, 3 Apr 2024 18:14:39 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> Creating a link to a constructor or a method or comparing constructors or >> methods __does not__ factor in type parameters. When constructors or methods >> are overloaded and differ only in type parameters -- a situation which is >> absent in JDK API, but present elsewhere -- that causes significant defects, >> such as: >> >> - missing entries in summary tables, lists and indexes, >> - duplicating links in the table of contents. >> >> This PR fixes those defects, and the fix is two-fold. Firstly, we update >> comparators to consider type parameters. That takes care of missing >> constructors and methods. Secondly, we update id (anchor) and link >> generation to always use the "erased" notation. That takes care of >> duplicating links. >> >> What's the "erased" notation? Suppose we have the following method: >> >> <T extends String> T m(T arg) >> >> The current notation refers to it as `m(T)`. That works fine until there's >> no other method, such as >> >> <T> T m(T arg) >> >> In which case, the current notation will produce a collision: `m(T)`. By >> contrast, the erased notation for those two methods is `m(java.lang.String)` >> and `m(java.lang.Object)` respectively. No collision. >> >> While longer, I believe that the erased notation is collision-proof. Why? >> Because [JLS 8.4.2][] says that "it is a compile-time error to declare two >> methods with override-equivalent signatures in a class". Which means that >> for any two constructors or methods the erasure of their signatures must >> differ, or else it won't compile. >> >> The change is pretty straightforward, except for some test fallout that >> required attention. >> >> [JLS 8.4.2]: >> https://docs.oracle.com/javase/specs/jls/se22/html/jls-8.html#jls-8.4.2 > > Pavel Rappo has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 11 additional > commits since the last revision: > > - Merge branch 'master' into 8325088 > - Remove registration phase > > Makes the code more robust and simple. > - Merge branch 'master' into 8325088 > - Update copyright years > > Note: any commit hashes below might be outdated due to subsequent > history rewriting (e.g. git rebase). > > - revert > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIndexBuilder.java > as spurious > - revert > test/langtools/jdk/javadoc/doclet/testMemberInheritance/TestMemberInheritance.java > as spurious > - revert > test/langtools/jdk/javadoc/doclet/testMemberSummary/TestMemberSummary.java as > spurious > - revert > test/langtools/jdk/javadoc/doclet/testNewLanguageFeatures/TestNewLanguageFeatures.java > as spurious > - revert test/langtools/jdk/javadoc/doclet/testOrdering/TestOrdering.java > as spurious > - revert > test/langtools/jdk/javadoc/doclet/testOverriddenMethods/TestOverrideMethods.java > as spurious > - revert > test/langtools/jdk/javadoc/doclet/testPrivateClasses/TestPrivateClasses.java > as spurious > - revert test/langtools/jdk/javadoc/doclet/testSeeTag/TestSeeTag.java as > spurious > - revert > test/langtools/jdk/javadoc/doclet/testVisibleMembers/TestVisibleMembers.java > as spurious > - Use erased notation only when necessary > > Partially reverts 4f028269, which is the bulk of the previous solution, > then adds a centralised ID registry for executable elements. > > The centralised registry is an alternative solution, which is more > gentle and less disruptive to tests and composability (-link and > -linkoffline). > - Update copyright years > > Note: any commit hashes below might be outdated due to subsequent > history rewriting (e.g. git rebase). > > + update > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIndexBuilder.java > due to 4f0282694fd > + update > test/langtools/jdk/javadoc/doclet/testMemberInheritance/TestMemberInheritance.java > due to 4f0282694fd > + update > test/langtools/jdk/javadoc/doclet/testMemberSummary/TestMemberSummary.java > due to 4f0282694fd > + update > test/langtools/jdk/javadoc/doclet/testNewLanguageFeatures/TestNewLanguageFeatures.java > due to 4f0282694fd > + update test/langtools/jdk/javadoc/doclet/testOrdering/TestOrdering.... Chen, Jon, I changed the solution to be less disruptive. Erased id is now used only when necessary (which is never in the current JDK API Documentation or JDK tests that predate this PR). To determine if it is necessary to use the erased id for a constructor or method, simple ids for all executable elements of that type element are computed in order. If the simple id for an executable element `e` clashes with the simple id of some executable element computed before, the erased id for `e` is used. This seems to work just fine. As a bonus, the overall size of the PR has been reduced. In addition to usual checks, I performed a few more. Here's the result: - there's no change in the JDK API documentation generated by `make docs-jdk-api` - there's no change in the output of tests generated by `jtreg -ea -esa -J-Djavatest.maxOutputSize=999999999 -retain:all <more options> test/langtools:langtools_javadoc`; the only difference was observed and expected in `testRedirectLinks`, which use ephemeral ports that leak to URIs - there's no significant difference in time it takes to generate either of the above ------------- PR Comment: https://git.openjdk.org/jdk/pull/18519#issuecomment-2035287644
