Integrated: JDK-8287524: Improve checkboxes to select releases on deprecated API page
On Wed, 1 Jun 2022 14:46:31 GMT, Hannes Wallnöfer wrote: > The original purpose of this issue was to fix the problems of elements on the > deprecated API list that can't be deselected because their release is not > included in the list of checkboxes at the top of the page. This is fixed here > by adding a checkbox for all elements/releases not included in the list if > necessary. > > While working on it I discovered a few corner cases that produce undesirable > output for the deprecated list. This includes the case of a single release > (where a checkbox doesn't make any sense) and the case of some or all > releases not having any occurrences in the `since` element of deprecated > elements (in which case the checkbox is useless as well). A more detailed > description of these cases is available in the [JBS > issue](https://bugs.openjdk.java.net/browse/JDK-8287524). > > As examples for the output generated with this change I uploaded JDK API docs: > > http://cr.openjdk.java.net/~hannesw/8287524/api.00/deprecated-list.html > > To illustrate behaviour in corner cases mentioned above I also uploaded > deprecated pages generated by the `TestNewAPIListWriter` test (which despite > its name also tests the "extended" deprecated list). > > http://cr.openjdk.java.net/~hannesw/8287524/test.00/TestNewApiList/out-multi/deprecated-list.html > http://cr.openjdk.java.net/~hannesw/8287524/test.00/TestNewApiList/out-none/deprecated-list.html > http://cr.openjdk.java.net/~hannesw/8287524/test.00/TestNewApiList/out-package/deprecated-list.html > http://cr.openjdk.java.net/~hannesw/8287524/test.00/TestNewApiList/out-single/deprecated-list.html This pull request has now been integrated. Changeset: 4d6fb515 Author:Hannes Wallnöfer URL: https://git.openjdk.java.net/jdk/commit/4d6fb515516dfc5f04c9d8e9c3602292e89df52a Stats: 166 lines in 9 files changed: 62 ins; 28 del; 76 mod 8287524: Improve checkboxes to select releases on deprecated API page Reviewed-by: jjg - PR: https://git.openjdk.java.net/jdk/pull/8973
Re: RFR: JDK-8287674: CSS improvements for summary lists [v2]
> This is a refinement of CSS styles mostly in the deprecated and new API pages. > > - Provide a two-row table view for the new sortable three-column tables > - Make sure no line wraps are inserted between a checkbox and its label > - Slightly increase the line height of the check box row to make it easier > to use when wrapped > - Define the overflow of summary table cells as "auto" which makes them > scrollable if necessary Hannes Wallnöfer 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8975/files - new: https://git.openjdk.java.net/jdk/pull/8975/files/d595fd7c..d595fd7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8975&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8975&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8975.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8975/head:pull/8975 PR: https://git.openjdk.java.net/jdk/pull/8975
Re: RFR: JDK-8287674: CSS improvements for summary lists
On Wed, 1 Jun 2022 16:16:33 GMT, Hannes Wallnöfer wrote: > This is a refinement of CSS styles mostly in the deprecated and new API pages. > > - Provide a two-row table view for the new sortable three-column tables > - Make sure no line wraps are inserted between a checkbox and its label > - Slightly increase the line height of the check box row to make it easier > to use when wrapped > - Define the overflow of summary table cells as "auto" which makes them > scrollable if necessary The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout JDK-8287674 git fetch https://git.openjdk.java.net/jdk master git merge FETCH_HEAD # if there are conflicts, follow the instructions given by git merge git commit -m "Merge master" git push - PR: https://git.openjdk.java.net/jdk/pull/8975
RFR: 8286206: Missing cases for RECORD
Adds handling of `ElementKind.RECORD` to JShell's code completion. - Commit messages: - 8286206: Missing cases for RECORD Changes: https://git.openjdk.java.net/jdk/pull/9042/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=9042&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286206 Stats: 40 lines in 3 files changed: 31 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/9042.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9042/head:pull/9042 PR: https://git.openjdk.java.net/jdk/pull/9042
Re: RFR: 8286206: Missing cases for RECORD
On Mon, 6 Jun 2022 14:40:01 GMT, Jan Lahoda wrote: > Adds handling of `ElementKind.RECORD` to JShell's code completion. I can't easily verify whether you have found all the necessary places to update, but I do understand/approve what I see here. On switch statements, like at JavadocHelper:701, would it help to list the cases now covered by default, and use `default` as an "error" to catch unhandled new cases in future? - Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9042
Re: RFR: 8286206: Missing cases for RECORD
On Mon, 6 Jun 2022 14:40:01 GMT, Jan Lahoda wrote: > Adds handling of `ElementKind.RECORD` to JShell's code completion. looks sensible to me - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9042
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Sun, 5 Jun 2022 20:55:12 GMT, Pavel Rappo wrote:
>> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
>> copies one instance of the specified exception.
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 33 commits:
>
> - Merge branch 'master' into 8287333
>
>This resolves a conflict in ParamTaglet.
> - Clean up if-branch
> - Remove upper-bounded wildcard
>
>This change simplifies code without any disadvantages:
>
> * Those `List` are read-only
> * An argument of the `List` type can still be passed to a `List extends XTree>` parameter
> - Simplify inheritThrowsDocumentation
> - Reuse more specific variable
> - Merge branch 'master' into 8287333
> - Incremental update
>
>- Renames local variables and method parameters
>- Improves comments
>- Removes debug leftovers
> - Update top-level doc comment
> - Trivially re-order assignments
>
>...for re-use
> - Reformat for clarity
>
>Now it's very clear that the "Throws:" section consists of three types of
> exceptions:
>
> 1. documented
> 2. inherited
> 3. undocumented
> - ... and 23 more:
> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
Finished reviewing May 25 commits.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 70:
> 68: */
> 69: private static Map mapNameToPosition(Utils utils,
> List params) {
> 70: Map result = new HashMap<>();
Is there a reason not to change this to `Map`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 94:
> 92: ? ee.getTypeParameters()
> 93: : ee.getParameters();
> 94: String target = ch.getParameterName((ParamTree)
> input.docTreeInfo.docTree());
I'm guessing that it is not practical to have DocTreeInfo be generic in the
DocTree field.
An alternative coding style here is to pass in an argument for the expected
return type, as in
String target =
ch.getParameterName(input.docTreeInfo.docTree(ParamTree.class));
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 108:
> 106: ExecutableElement md = (ExecutableElement) input.element;
> 107: CommentHelper ch = utils.getCommentHelper(md);
> 108: List tags = input.isTypeVariableParamTag
This sort of cleanup is nice, and welcome.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 147:
> 145: /**
> 146: * Returns a {@code Content} representation of a list of {@code
> ParamTree}.
> 147: * Tries to inherit the param tags that are missing.
mild grumble to see information being removed, even it is obvious to you.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 149:
> 147: * Tries to inherit the param tags that are missing.
> 148: */
> 149: private Content getTagletOutput(Element holder, ParamKind kind,
It's not wrong, but I don't see the reason for rearranging the parameter list
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 177:
> 175: Content result = writer.getOutputInstance();
> 176: Input input = new DocFinder.Input(writer.configuration().utils,
> holder, this,
> 177: Integer.toString(position), kind ==
> ParamKind.TYPE_PARAMETER);
Inconsistent w.r.t line 76 ... `String.valueOf` vs `Integer.toString`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 194:
> 192: * Returns a {@code Content} representation of a list of {@code
> ParamTree}.
> 193: *
> 194: * Warns about {@code @param} tags that do not map to parameter
> elements
It does more than just warn ...
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 209:
> 207: List
> formalParameters,
> 208: TagletWriter writer) {
> 209: Map tagOfPosition = new HashMap<>();
I still think that one of the type parameters should be `Integer`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 281:
> 279: * @param namethe name of the parameter. We can't rely
> on
> 280: *the name in the param tag because we might
> be
> 281: *inheriting documentation.
This is useful info you are deleting :-(
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
line 56:
> 54:
> 55: /**
> 56: * A taglet that processes {@link ThrowsTree}, which represents tags like
rephrase without "like"
src/jdk.javadoc/share/classes/jdk/j
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Sun, 5 Jun 2022 20:55:12 GMT, Pavel Rappo wrote:
>> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
>> copies one instance of the specified exception.
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 33 commits:
>
> - Merge branch 'master' into 8287333
>
>This resolves a conflict in ParamTaglet.
> - Clean up if-branch
> - Remove upper-bounded wildcard
>
>This change simplifies code without any disadvantages:
>
> * Those `List` are read-only
> * An argument of the `List` type can still be passed to a `List extends XTree>` parameter
> - Simplify inheritThrowsDocumentation
> - Reuse more specific variable
> - Merge branch 'master' into 8287333
> - Incremental update
>
>- Renames local variables and method parameters
>- Improves comments
>- Removes debug leftovers
> - Update top-level doc comment
> - Trivially re-order assignments
>
>...for re-use
> - Reformat for clarity
>
>Now it's very clear that the "Throws:" section consists of three types of
> exceptions:
>
> 1. documented
> 2. inherited
> 3. undocumented
> - ... and 23 more:
> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
line 206:
> 204: // Of these two, only methods inherit documentation.
> 205: // Don't waste time on constructors.
> 206: assert holder.getKind() == ElementKind.CONSTRUCTOR :
> holder.getKind();
This is weaker than the old code, which would throw an Error on other types.
`assert` may or may not be enabled.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Sun, 5 Jun 2022 20:55:12 GMT, Pavel Rappo wrote:
>> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
>> copies one instance of the specified exception.
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 33 commits:
>
> - Merge branch 'master' into 8287333
>
>This resolves a conflict in ParamTaglet.
> - Clean up if-branch
> - Remove upper-bounded wildcard
>
>This change simplifies code without any disadvantages:
>
> * Those `List` are read-only
> * An argument of the `List` type can still be passed to a `List extends XTree>` parameter
> - Simplify inheritThrowsDocumentation
> - Reuse more specific variable
> - Merge branch 'master' into 8287333
> - Incremental update
>
>- Renames local variables and method parameters
>- Improves comments
>- Removes debug leftovers
> - Update top-level doc comment
> - Trivially re-order assignments
>
>...for re-use
> - Reformat for clarity
>
>Now it's very clear that the "Throws:" section consists of three types of
> exceptions:
>
> 1. documented
> 2. inherited
> 3. undocumented
> - ... and 23 more:
> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
Finished reviewing.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
line 217:
> 215: if (inheritedDoc.tagList.isEmpty()) {
> 216: input = new DocFinder.Input(utils, holder, this,
> 217: utils.getTypeName(declaredExceptionType, true));
I guess I don't understand your coding style, because elsewhere (earlier) in
this review, you factored out sub-expressions into their own `var` variable.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v3]
On Fri, 3 Jun 2022 14:47:13 GMT, Pavel Rappo wrote:
>> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
>> copies one instance of the specified exception.
>
> Pavel Rappo has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Remove upper-bounded wildcard
>
>This change simplifies code without any disadvantages:
>
> * Those `List` are read-only
> * An argument of the `List` type can still be passed to a `List extends XTree>` parameter
> - Simplify inheritThrowsDocumentation
> - Reuse more specific variable
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 92:
> 90: ? utils.getTypeParamTrees(ee)
> 91: : utils.getParamTrees(ee);
> 92: List parameters = input.isTypeVariableParamTag
Change both or neither; don't leave the code inconsistent
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
line 164:
> 162: ParamKind kind,
> 163: List tags,
> 164: List parameters,
visually inconsistent
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:22:34 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 33 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - Trivially re-order assignments
>>
>>...for re-use
>> - Reformat for clarity
>>
>>Now it's very clear that the "Throws:" section consists of three types of
>> exceptions:
>>
>> 1. documented
>> 2. inherited
>> 3. undocumented
>> - ... and 23 more:
>> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 70:
>
>> 68: */
>> 69: private static Map mapNameToPosition(Utils utils,
>> List params) {
>> 70: Map result = new HashMap<>();
>
> Is there a reason not to change this to `Map`
We could change it to `Integer` now, but it wouldn't help us much: the
`DocFinder.Input.tagId` field, which that `Integer` is ultimately compared
against, is of type `String`. To make it all the way, we would need to change
`tagId` to `Integer` too. But that won't work because `tagId` is also used by
`ThrowsTaglet` to store an exception class (or an exception type variable) name.
That said, I agree that we should change `String` to `Integer` there. Let's
just not do it now as it requires deeper refactoring; I have a plan already and
some implementation in the works.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:24:08 GMT, Jonathan Gibbons wrote: >> Pavel Rappo has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 33 commits: >> >> - Merge branch 'master' into 8287333 >> >>This resolves a conflict in ParamTaglet. >> - Clean up if-branch >> - Remove upper-bounded wildcard >> >>This change simplifies code without any disadvantages: >> >> * Those `List` are read-only >> * An argument of the `List` type can still be passed to a >> `List` parameter >> - Simplify inheritThrowsDocumentation >> - Reuse more specific variable >> - Merge branch 'master' into 8287333 >> - Incremental update >> >>- Renames local variables and method parameters >>- Improves comments >>- Removes debug leftovers >> - Update top-level doc comment >> - Trivially re-order assignments >> >>...for re-use >> - Reformat for clarity >> >>Now it's very clear that the "Throws:" section consists of three types of >> exceptions: >> >> 1. documented >> 2. inherited >> 3. undocumented >> - ... and 23 more: >> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java > line 177: > >> 175: Content result = writer.getOutputInstance(); >> 176: Input input = new DocFinder.Input(writer.configuration().utils, >> holder, this, >> 177: Integer.toString(position), kind == >> ParamKind.TYPE_PARAMETER); > > Inconsistent w.r.t line 76 ... `String.valueOf` vs `Integer.toString` Huh? 02021a27 changed all `String.valueOf` to `Integer.toString` for consistency. - PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:27:07 GMT, Jonathan Gibbons wrote:
> It does more than just warn ...
What else beyond converting `ParamTree`s to `Content` and warning authors about
unexpected `ParamTree` does it do?
Separately. Could it be that you haven't seen further commits? GitHub does not
seem to indicate to me which commits your comments refer to.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 209:
>
>> 207: List
>> formalParameters,
>> 208: TagletWriter writer) {
>> 209: Map tagOfPosition = new HashMap<>();
>
> I still think that one of the type parameters should be `Integer`
See my earlier comment on `Integer`, `tagId` and `ThrowsTaglet`.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:39:10 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 33 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - Trivially re-order assignments
>>
>>...for re-use
>> - Reformat for clarity
>>
>>Now it's very clear that the "Throws:" section consists of three types of
>> exceptions:
>>
>> 1. documented
>> 2. inherited
>> 3. undocumented
>> - ... and 23 more:
>> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 147:
>
>> 145: /**
>> 146: * Returns a {@code Content} representation of a list of {@code
>> ParamTree}.
>> 147: * Tries to inherit the param tags that are missing.
>
> mild grumble to see information being removed, even it is obvious to you.
I can revert that, if you want.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:50:05 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 33 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - Trivially re-order assignments
>>
>>...for re-use
>> - Reformat for clarity
>>
>>Now it's very clear that the "Throws:" section consists of three types of
>> exceptions:
>>
>> 1. documented
>> 2. inherited
>> 3. undocumented
>> - ... and 23 more:
>> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 90:
>
>> 88: output.tagList.add(tag);
>> 89: } else if (target != null && candidate != null &&
>> 90: utils.isTypeElement(candidate) &&
>> utils.isTypeElement(target) && // FIXME: can they be anything else other
>> than type elements?
>
> what about `TypeParameterElement`?
I deleted that FIXME in a later commit. Also, that particular if-branch is to
be looked at closely in JDK-8287796.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 219:
>
>> 217: .map(t -> (ThrowsTree) t)
>> 218: .toList();
>> 219: ExecutableElement r =
>> declaredExceptionTags.put(inheritedTags, (ExecutableElement)
>> inheritedDoc.holder);
>
> I do not understand the need for saving the result
That was removed in a later commit, 6d51f62a.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 237:
>
>> 235: Utils utils = writer.configuration().utils;
>> 236: Content result = writer.getOutputInstance();
>> 237: for (TypeMirror declaredExceptionType : declaredExceptionTypes)
>> {
>
> Two comments may have been too many, but zero is too few.
Did you see the name of that private method? Looks pretty self-descriptive to
me. I'm not sure why you think it needs any comment, but I can re-introduce it,
if you wish so.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v3]
On Mon, 6 Jun 2022 21:27:25 GMT, Jonathan Gibbons wrote: >> Pavel Rappo has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Remove upper-bounded wildcard >> >>This change simplifies code without any disadvantages: >> >> * Those `List` are read-only >> * An argument of the `List` type can still be passed to a >> `List` parameter >> - Simplify inheritThrowsDocumentation >> - Reuse more specific variable > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java > line 92: > >> 90: ? utils.getTypeParamTrees(ee) >> 91: : utils.getParamTrees(ee); >> 92: List parameters = input.isTypeVariableParamTag > > Change both or neither; don't leave the code inconsistent I'm not sure why list of parameter trees have to be the same as list of elements. I can change (methods such as) `Utils#getTypeParamTrees`, but I cannot change (methods such as) `ExecutableElement#getTypeParameters` (sadly). Useless bounded wildcards clutter code. - PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 20:43:11 GMT, Jonathan Gibbons wrote:
>> Pavel Rappo has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 33 commits:
>>
>> - Merge branch 'master' into 8287333
>>
>>This resolves a conflict in ParamTaglet.
>> - Clean up if-branch
>> - Remove upper-bounded wildcard
>>
>>This change simplifies code without any disadvantages:
>>
>> * Those `List` are read-only
>> * An argument of the `List` type can still be passed to a
>> `List` parameter
>> - Simplify inheritThrowsDocumentation
>> - Reuse more specific variable
>> - Merge branch 'master' into 8287333
>> - Incremental update
>>
>>- Renames local variables and method parameters
>>- Improves comments
>>- Removes debug leftovers
>> - Update top-level doc comment
>> - Trivially re-order assignments
>>
>>...for re-use
>> - Reformat for clarity
>>
>>Now it's very clear that the "Throws:" section consists of three types of
>> exceptions:
>>
>> 1. documented
>> 2. inherited
>> 3. undocumented
>> - ... and 23 more:
>> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
> line 149:
>
>> 147: * Tries to inherit the param tags that are missing.
>> 148: */
>> 149: private Content getTagletOutput(Element holder, ParamKind kind,
>
> It's not wrong, but I don't see the reason for rearranging the parameter list
That way the parameter kind is adjacent to parameters themselves in the list of
arguments:
ParamKind kind,
List tags,
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 206:
>
>> 204: // Of these two, only methods inherit documentation.
>> 205: // Don't waste time on constructors.
>> 206: assert holder.getKind() == ElementKind.CONSTRUCTOR :
>> holder.getKind();
>
> This is weaker than the old code, which would throw an Error on other types.
> `assert` may or may not be enabled.
Hm... Which kind of "Error" would be thrown for which "other type"? I don't
think that this method is ever called with an element other than a constructor
or method; look:
public ThrowsTaglet() {
super(DocTree.Kind.THROWS, false, EnumSet.of(Location.CONSTRUCTOR,
Location.METHOD));
}
It is called for constructors regularly, but as you might imagine, it has no
effect other than waste of resources and, what is more important, loss of code
clarity. There are multiple types of executable elements. It's both clarifying
and reassuring to see an assertion that establishes our programming assumptions
as to which of those types we expect there, early in the method.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
> line 217:
>
>> 215: if (inheritedDoc.tagList.isEmpty()) {
>> 216: input = new DocFinder.Input(utils, holder, this,
>> 217: utils.getTypeName(declaredExceptionType, true));
>
> I guess I don't understand your coding style, because elsewhere (earlier) in
> this review, you factored out sub-expressions into their own `var` variable.
It was later fixed in 40ca8088.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Tue, 7 Jun 2022 01:10:24 GMT, Pavel Rappo wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
>> line 206:
>>
>>> 204: // Of these two, only methods inherit documentation.
>>> 205: // Don't waste time on constructors.
>>> 206: assert holder.getKind() == ElementKind.CONSTRUCTOR :
>>> holder.getKind();
>>
>> This is weaker than the old code, which would throw an Error on other types.
>> `assert` may or may not be enabled.
>
> Hm... Which kind of "Error" would be thrown for which "other type"? I don't
> think that this method is ever called with an element other than a
> constructor or method; look:
>
> public ThrowsTaglet() {
> super(DocTree.Kind.THROWS, false, EnumSet.of(Location.CONSTRUCTOR,
> Location.METHOD));
> }
>
> It is called for constructors regularly, but as you might imagine, it has no
> effect other than waste of resources and, what is more important, loss of
> code clarity. There are multiple types of executable elements. It's both
> clarifying and reassuring to see an assertion that establishes our
> programming assumptions as to which of those types we expect there, early in
> the method.
I continue to think that `assert` is little better than a stylized comment,
since they are typically not enabled.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: JDK-8287674: CSS improvements for summary lists [v3]
> This is a refinement of CSS styles mostly in the deprecated and new API pages. > > - Provide a two-row table view for the new sortable three-column tables > - Make sure no line wraps are inserted between a checkbox and its label > - Slightly increase the line height of the check box row to make it easier > to use when wrapped > - Define the overflow of summary table cells as "auto" which makes them > scrollable if necessary Hannes Wallnöfer has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Merge branch 'master' into JDK-8287674 - JDK-8287674: CSS improvements for summary lists - Add comment - JDK-8287524: Improve checkboxes to select releases on deprecated API page - Changes: https://git.openjdk.java.net/jdk/pull/8975/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8975&range=02 Stats: 36 lines in 5 files changed: 16 ins; 4 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/8975.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8975/head:pull/8975 PR: https://git.openjdk.java.net/jdk/pull/8975
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Tue, 7 Jun 2022 00:26:13 GMT, Pavel Rappo wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ThrowsTaglet.java
>> line 237:
>>
>>> 235: Utils utils = writer.configuration().utils;
>>> 236: Content result = writer.getOutputInstance();
>>> 237: for (TypeMirror declaredExceptionType :
>>> declaredExceptionTypes) {
>>
>> Two comments may have been too many, but zero is too few.
>
> Did you see the name of that private method? Looks pretty self-descriptive to
> me. I'm not sure why you think it needs any comment, but I can re-introduce
> it, if you wish so.
Yeah, the name is not great, and that shows up when you see the 3 groups of
exceptions side by side lower down.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v3]
On Tue, 7 Jun 2022 00:35:56 GMT, Pavel Rappo wrote: >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java >> line 92: >> >>> 90: ? utils.getTypeParamTrees(ee) >>> 91: : utils.getParamTrees(ee); >>> 92: List parameters = >>> input.isTypeVariableParamTag >> >> Change both or neither; don't leave the code inconsistent > > I'm not sure why list of parameter trees have to be the same as list of > elements. I can change (methods such as) `Utils#getTypeParamTrees`, but I > cannot change (methods such as) `ExecutableElement#getTypeParameters` > (sadly). Useless bounded wildcards clutter code. Hmm, not really convinced but OK - PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 23:31:19 GMT, Pavel Rappo wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
>> line 70:
>>
>>> 68: */
>>> 69: private static Map mapNameToPosition(Utils utils,
>>> List params) {
>>> 70: Map result = new HashMap<>();
>>
>> Is there a reason not to change this to `Map`
>
> We could change it to `Integer` now, but it wouldn't help us much: the
> `DocFinder.Input.tagId` field, which that `Integer` is ultimately compared
> against, is of type `String`. To make it all the way, we would need to change
> `tagId` to `Integer` too. But that won't work because `tagId` is also used by
> `ThrowsTaglet` to store an exception class (or an exception type variable)
> name.
>
> That said, I agree that we should change `String` to `Integer` there. Let's
> just not do it now as it requires deeper refactoring; I have a plan already
> and some implementation in the works.
Noted about tagId. You could still do a local cleanup for the type of the map,
but I guess we'll wait and see what your plan is.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Mon, 6 Jun 2022 23:46:05 GMT, Pavel Rappo wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/ParamTaglet.java
>> line 209:
>>
>>> 207: List
>>> formalParameters,
>>> 208: TagletWriter writer) {
>>> 209: Map tagOfPosition = new HashMap<>();
>>
>> I still think that one of the type parameters should be `Integer`
>
> See my earlier comment on `Integer`, `tagId` and `ThrowsTaglet`.
Noted.
-
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: 8287333: Clean up ParamTaglet and ThrowsTaglet [v4]
On Sun, 5 Jun 2022 20:55:12 GMT, Pavel Rappo wrote:
>> A cleanup to facilitate fixing bugs like JDK-6509045: `{@inheritDoc}` only
>> copies one instance of the specified exception.
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 33 commits:
>
> - Merge branch 'master' into 8287333
>
>This resolves a conflict in ParamTaglet.
> - Clean up if-branch
> - Remove upper-bounded wildcard
>
>This change simplifies code without any disadvantages:
>
> * Those `List` are read-only
> * An argument of the `List` type can still be passed to a `List extends XTree>` parameter
> - Simplify inheritThrowsDocumentation
> - Reuse more specific variable
> - Merge branch 'master' into 8287333
> - Incremental update
>
>- Renames local variables and method parameters
>- Improves comments
>- Removes debug leftovers
> - Update top-level doc comment
> - Trivially re-order assignments
>
>...for re-use
> - Reformat for clarity
>
>Now it's very clear that the "Throws:" section consists of three types of
> exceptions:
>
> 1. documented
> 2. inherited
> 3. undocumented
> - ... and 23 more:
> https://git.openjdk.java.net/jdk/compare/ebc012ec...6bbe871b
OK, but it's a big blob of change; it's a shame it could not have been
partitioned more.
-
Marked as reviewed by jjg (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8886
Re: RFR: JDK-8287674: CSS improvements for summary lists [v4]
> This is a refinement of CSS styles mostly in the deprecated and new API pages. > > - Provide a two-row table view for the new sortable three-column tables > - Make sure no line wraps are inserted between a checkbox and its label > - Slightly increase the line height of the check box row to make it easier > to use when wrapped > - Define the overflow of summary table cells as "auto" which makes them > scrollable if necessary Hannes Wallnöfer has updated the pull request incrementally with two additional commits since the last revision: - Remove unused imports - Use normal space instead of as the latter is not rendered as space by Chrome - Changes: - all: https://git.openjdk.java.net/jdk/pull/8975/files - new: https://git.openjdk.java.net/jdk/pull/8975/files/2bed76c1..7ae1fc73 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8975&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8975&range=02-03 Stats: 16 lines in 3 files changed: 0 ins; 2 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/8975.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8975/head:pull/8975 PR: https://git.openjdk.java.net/jdk/pull/8975
Re: RFR: JDK-8287674: CSS improvements for summary lists [v4]
On Tue, 7 Jun 2022 06:21:59 GMT, Hannes Wallnöfer wrote: >> This is a refinement of CSS styles mostly in the deprecated and new API >> pages. >> >> - Provide a two-row table view for the new sortable three-column tables >> - Make sure no line wraps are inserted between a checkbox and its label >> - Slightly increase the line height of the check box row to make it easier >> to use when wrapped >> - Define the overflow of summary table cells as "auto" which makes them >> scrollable if necessary > > Hannes Wallnöfer has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove unused imports > - Use normal space instead of as the latter is not rendered as space > by Chrome I have replaced the `` elements between checkboxes with plain space characters as the former reduces the spacing between checkboxes on Chrome. With plain space characters, spacing is the same as before on all tested browsers and line wrapping works as expected. - PR: https://git.openjdk.java.net/jdk/pull/8975
Integrated: JDK-8287674: CSS improvements for summary lists
On Wed, 1 Jun 2022 16:16:33 GMT, Hannes Wallnöfer wrote: > This is a refinement of CSS styles mostly in the deprecated and new API pages. > > - Provide a two-row table view for the new sortable three-column tables > - Make sure no line wraps are inserted between a checkbox and its label > - Slightly increase the line height of the check box row to make it easier > to use when wrapped > - Define the overflow of summary table cells as "auto" which makes them > scrollable if necessary This pull request has now been integrated. Changeset: f6a79e34 Author:Hannes Wallnöfer URL: https://git.openjdk.java.net/jdk/commit/f6a79e340fb7545ab3ab75ea7da3d9a1a368b46f Stats: 34 lines in 5 files changed: 14 ins; 4 del; 16 mod 8287674: CSS improvements for summary lists Reviewed-by: jjg - PR: https://git.openjdk.java.net/jdk/pull/8975
