Re: RFR: 8248001: javadoc generates invalid HTML pages whose ftp:// links are broken [v4]
> Could you please review the 8248001 bug fixes? > > The problem is that javadoc generates invalid HTML pages whose ftp:// links > are broken. The fix changes not to use protocol names directly, but to use > regular expression. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8248001: javadoc generates invalid HTML pages whose ftp:// links are broken - Changes: - all: https://git.openjdk.java.net/jdk/pull/5198/files - new: https://git.openjdk.java.net/jdk/pull/5198/files/f336b867..b68c760b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5198&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5198&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5198.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5198/head:pull/5198 PR: https://git.openjdk.java.net/jdk/pull/5198
Re: RFR: 8248001: javadoc generates invalid HTML pages whose ftp:// links are broken [v4]
On Tue, 28 Sep 2021 08:23:30 GMT, Masanori Yano wrote: >> Could you please review the 8248001 bug fixes? >> >> The problem is that javadoc generates invalid HTML pages whose ftp:// links >> are broken. The fix changes not to use protocol names directly, but to use >> regular expression. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8248001: javadoc generates invalid HTML pages whose ftp:// links are broken I updated the copyright date to 2021. I think `checkHtml` is not necessary because `JavadocTester` checks links by `checkLinks()`. The following is log when the fix has not been applyed. Checked 11 files. Found 189 references to 54 anchors in 16 files and 6 other URIs. * 1 missing files 0 duplicate ids 0 missing ids Schemes 1 file 1 ftp 1 http 2 https 1 mailto Hosts 1 www.example.com 2 docs.oracle.com 1 www.exsample.com FAILED: 1 errors found when checking links at javadoc.tester.JavadocTester.checkLinks(JavadocTester.java:565) at javadoc.tester.JavadocTester.javadoc(JavadocTester.java:384) at TestHrefInDocComment.test(TestHrefInDocComment.java:46) - PR: https://git.openjdk.java.net/jdk/pull/5198
Re: RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields [v3]
> This is an initial PR for expanded lint warnings done under two bugs:
>
> 8202056: Expand serial warning to check for bad overloads of serial-related
> methods and ineffectual fields
> 8160675: Issue lint warning for non-serializable non-transient instance
> fields in serializable type
>
> to get feedback on the general approach and test strategy before further
> polishing the implementation.
>
> The implementation initially started as an annotation processor I wrote
> several years ago. The refined version being incorporated into Attr has been
> refactored, had its checks expanded, and been partially ported to idiomatic
> javac coding style rather than using the javax.lang.model API from annotation
> processing.
>
> Subsequent versions of this PR are expected to move the implementation closer
> to idiomatic javac, in particular to use javac flags rather than
> javax.lang.model.Modifier's. Additional resources keys will be defined for
> the serialization-related fields and methods not having the expected
> modifiers, types, etc. The resource keys for the existing checks related to
> serialVersionUID and reused.
>
> Please also review the corresponding CSRs:
>
> https://bugs.openjdk.java.net/browse/JDK-8274335
> https://bugs.openjdk.java.net/browse/JDK-8274336
>
> Informative serialization-related warning messages must take into account
> whether a class, interface, annotation, record, and enum is being analyzed.
> Enum classes and record classes have special handling in serialization. This
> implementation under review has been augmented with checks for interface
> types recommended by Chris Hegarty in an attachment on 8202056.
>
> The JDK build has the Xlint:serial check enabled. The build did not pass with
> the augmented checks. For most modules, this PR contains the library changes
> necessary for the build to pass. I will start separate PRs in those library
> areas to get the needed SuppressWarning("serial") or other changes in place.
> For one module, I temporarily disabled the Xlint:serial check.
>
> In terms of performance, I have not done benchmarks of the JDK build with and
> without these changes, but informally the build seems to take about as long
> as before.
Joe Darcy 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 33 additional commits since the
last revision:
- Merge branch 'master' into JDK-8202056
- Merge branch 'master' into JDK-8202056
- Restore serial checks on java.xml module.
- Merge branch 'master' into JDK-8202056
- Appease jcheck
- Implement checks chegar recommended for interfaces.
- Update comment.
- Add tests for instance field checks.
- Clean build with instance field checks in place.
- Merge branch 'master' into JDK-8202056
- ... and 23 more: https://git.openjdk.java.net/jdk/compare/7925d0cc...bacff4e9
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5709/files
- new: https://git.openjdk.java.net/jdk/pull/5709/files/053de6bb..bacff4e9
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5709&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5709&range=01-02
Stats: 2951 lines in 77 files changed: 2399 ins; 397 del; 155 mod
Patch: https://git.openjdk.java.net/jdk/pull/5709.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5709/head:pull/5709
PR: https://git.openjdk.java.net/jdk/pull/5709
Re: RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields [v4]
> This is an initial PR for expanded lint warnings done under two bugs:
>
> 8202056: Expand serial warning to check for bad overloads of serial-related
> methods and ineffectual fields
> 8160675: Issue lint warning for non-serializable non-transient instance
> fields in serializable type
>
> to get feedback on the general approach and test strategy before further
> polishing the implementation.
>
> The implementation initially started as an annotation processor I wrote
> several years ago. The refined version being incorporated into Attr has been
> refactored, had its checks expanded, and been partially ported to idiomatic
> javac coding style rather than using the javax.lang.model API from annotation
> processing.
>
> Subsequent versions of this PR are expected to move the implementation closer
> to idiomatic javac, in particular to use javac flags rather than
> javax.lang.model.Modifier's. Additional resources keys will be defined for
> the serialization-related fields and methods not having the expected
> modifiers, types, etc. The resource keys for the existing checks related to
> serialVersionUID and reused.
>
> Please also review the corresponding CSRs:
>
> https://bugs.openjdk.java.net/browse/JDK-8274335
> https://bugs.openjdk.java.net/browse/JDK-8274336
>
> Informative serialization-related warning messages must take into account
> whether a class, interface, annotation, record, and enum is being analyzed.
> Enum classes and record classes have special handling in serialization. This
> implementation under review has been augmented with checks for interface
> types recommended by Chris Hegarty in an attachment on 8202056.
>
> The JDK build has the Xlint:serial check enabled. The build did not pass with
> the augmented checks. For most modules, this PR contains the library changes
> necessary for the build to pass. I will start separate PRs in those library
> areas to get the needed SuppressWarning("serial") or other changes in place.
> For one module, I temporarily disabled the Xlint:serial check.
>
> In terms of performance, I have not done benchmarks of the JDK build with and
> without these changes, but informally the build seems to take about as long
> as before.
Joe Darcy has updated the pull request with a new target base due to a merge or
a rebase. The pull request now contains 35 commits:
- Fix whitespace.
- Merge branch 'master' into JDK-8202056
- Merge branch 'master' into JDK-8202056
- Merge branch 'master' into JDK-8202056
- Restore serial checks on java.xml module.
- Merge branch 'master' into JDK-8202056
- Appease jcheck
- Implement checks chegar recommended for interfaces.
- Update comment.
- Add tests for instance field checks.
- ... and 25 more: https://git.openjdk.java.net/jdk/compare/d8a278f3...37ad8778
-
Changes: https://git.openjdk.java.net/jdk/pull/5709/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5709&range=03
Stats: 1495 lines in 64 files changed: 1488 ins; 0 del; 7 mod
Patch: https://git.openjdk.java.net/jdk/pull/5709.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5709/head:pull/5709
PR: https://git.openjdk.java.net/jdk/pull/5709
RFR: JDK-8273154: Provide a JavadocTester method for non-overlapping, unordered output matching
Please review a moderately simple improvement for `JavadocTester` and a related new test. A new `OutputChecker` class is introduced that mostly supersedes the existing methods to check the output generated by javadoc and the standard doclet. A self-imposed restriction is that no existing tests are modified. The new class can be used to check files generated by the doclet and the streams written by the tool. It can be configured to check for ordered output or not, overlapping output, and complete coverage, and can search for literal strings and regular expressions. There is a corresponding new test which is a non-standard use of `JavadocTester`, since it is designed to test `JavadocTester` itself, and not javadoc or the doclet. (Quis custodiet ipsos custodes?) Various methods are overridden so that the operation of the underlying methods can be checked. Although it is a goal to NOT modify the code of any existing tests, it turns out to be reasonable to adapt some of the existing `check...` methods to use the new `OutputChecker`. All javadoc tests pass, both locally and on all standard platforms. Many/most uses of the existing `checkOutput` method provide "ordered" strings, and are candidates to use the new ordered check. But enough uses are _not_ ordered, so it is not reasonable to change the default at this time. It is noted as a TODO to examine the appropriate test cases, so that we can decide whether to fix those tests and change the default. - Commit messages: - JDK-8273154: Provide a JavadocTester method for non-overlapping, unordered output matching Changes: https://git.openjdk.java.net/jdk/pull/5743/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5743&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273154 Stats: 811 lines in 2 files changed: 741 ins; 57 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/5743.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5743/head:pull/5743 PR: https://git.openjdk.java.net/jdk/pull/5743
Re: RFR: JDK-8273154: Provide a JavadocTester method for non-overlapping, unordered output matching
On Tue, 28 Sep 2021 23:34:21 GMT, Jonathan Gibbons wrote:
> Please review a moderately simple improvement for `JavadocTester` and a
> related new test.
>
> A new `OutputChecker` class is introduced that mostly supersedes the existing
> methods to check the output generated by javadoc and the standard doclet. A
> self-imposed restriction is that no existing tests are modified.
>
> The new class can be used to check files generated by the doclet and the
> streams written by the tool. It can be configured to check for ordered output
> or not, overlapping output, and complete coverage, and can search for literal
> strings and regular expressions.
>
> There is a corresponding new test which is a non-standard use of
> `JavadocTester`, since it is designed to test `JavadocTester` itself, and not
> javadoc or the doclet. (Quis custodiet ipsos custodes?) Various methods
> are overridden so that the operation of the underlying methods can be checked.
>
> Although it is a goal to NOT modify the code of any existing tests, it turns
> out to be reasonable to adapt some of the existing `check...` methods to use
> the new `OutputChecker`. All javadoc tests pass, both locally and on all
> standard platforms. Many/most uses of the existing `checkOutput` method
> provide "ordered" strings, and are candidates to use the new ordered check.
> But enough uses are _not_ ordered, so it is not reasonable to change the
> default at this time. It is noted as a TODO to examine the appropriate test
> cases, so that we can decide whether to fix those tests and change the
> default.
Reviewing the JBS issue, I see I did not address the following, which had been
my intent to do so. I will update the PR.
// TODO: perhaps this method could be added to JavadocTester
private void checkOutputEither(Output out, String first, String... other) {
checking("checkOutputEither");
String output = getOutput(out);
-
PR: https://git.openjdk.java.net/jdk/pull/5743
Integrated: JDK-8274172: Convert JavadocTester to use NIO
On Thu, 23 Sep 2021 01:44:27 GMT, Jonathan Gibbons wrote: > Please review a moderately simple update to convert JavadocTester to just use > NIO, instead of a mix of File and NIO. > > The original code used java.io.File. At some point (JDK 9-ish) new code was > added that used NIO, resulting in a mix. This change converts the old code to > use NIO as well. > > This is mostly internal, with two changes that affect tests. > > 1. The `protected` field `outputDir` is changed from a `File` to a `Path`. > Some tests use `outputDir` directly, typically to convert it to a `Path`. > 2. The `copyDir` method had a strange spec. Partly, it used "target" to > describe the directory being copied, but worse, it copied the entire source > directory INTO the destination directory, as compared to copying the > contents. The method was just used in a single test, so I've changed the > spec of the method and the use in the test. This cleaned up a "TODO" as > well, to use `Files.walkFileTree` for the copy. This pull request has now been integrated. Changeset: 5b0c9ccc Author:Jonathan Gibbons URL: https://git.openjdk.java.net/jdk/commit/5b0c9ccc495b6bf8061c161d48f73238c7167bc0 Stats: 207 lines in 8 files changed: 67 ins; 55 del; 85 mod 8274172: Convert JavadocTester to use NIO Reviewed-by: prappo - PR: https://git.openjdk.java.net/jdk/pull/5644
Re: RFR: JDK-8273244: Improve diagnostic output related to ErroneousTree [v3]
On Mon, 27 Sep 2021 15:49:25 GMT, Pavel Rappo wrote:
>> Jonathan Gibbons has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> address preliminary review comments
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 177:
>
>> 175: }
>> 176:
>> 177: case AUTHOR, DEPRECATED, HIDDEN, PARAM, RETURN, SEE,
>> SERIAL, SERIAL_DATA, SERIAL_FIELD, SINCE, THROWS, UNKNOWN_BLOCK_TAG, VERSION
>> -> {
>
> You added HIDDEN and merged PARAM into this case of block tags. Should we
> also add PROVIDES and USES? If so, then we should add respective tests.
Yes, and agreed.
-
PR: https://git.openjdk.java.net/jdk/pull/5510
Re: RFR: JDK-8273244: Improve diagnostic output related to ErroneousTree [v3]
On Mon, 27 Sep 2021 18:00:07 GMT, Pavel Rappo wrote:
> > Apart from fixing generally bad behavior, there is one other tiny
> > behavioral change. For an empty DocCommentTree the ending position is now
> > the same at the starting position, and not NOPOS.
>
> I suppose we don't need to reflect this change anywhere in the API (CSR), do
> we?
I don't believe the specification is so fine-grained.
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 174:
>
>> 172: case IDENTIFIER -> {
>> 173: DCIdentifier ident = (DCIdentifier) this;
>> 174: return ident.pos + ident.name.length();
>
> While in JavacTrees, this logic contained some special handling for
> `names.error`. What was that about?
`names.error` is used by javac in various error conditions. It has the string
value ``. I don't believe it can arise in `DocTree` nodes, so I dropped
the code.
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 192:
>
>> 190: case ENTITY -> {
>> 191: DCEntity endEl = (DCEntity) this;
>> 192: return endEl.pos + endEl.name.length() + 2;
>
> A similar question about `names.error`.
Similar answer.
-
PR: https://git.openjdk.java.net/jdk/pull/5510
Re: RFR: JDK-8273244: Improve diagnostic output related to ErroneousTree [v3]
On Mon, 27 Sep 2021 15:48:17 GMT, Pavel Rappo wrote:
>> Jonathan Gibbons has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> address preliminary review comments
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DCTree.java line 161:
>
>> 159: }
>> 160:
>> 161: switch (getKind()) {
>
> I can see that you changed this switch while moving it from JavacTrees.
Yes.
> src/jdk.compiler/share/classes/jdk/internal/shellsupport/doc/JavadocHelper.java
> line 498:
>
>> 496: //the newline:
>> 497: long endPos = sp.getEndPosition(null,
>> dcTree, tree);
>> 498: if (endPos >= offset) {
>
> What's that about?
Without the change, you can get an exception like
`StringIndexOutOfBoundsException` for a bad `endPos`.
I believe there's some underlying weirdness for synthesized HTML comments (i.e.
full `...` comments e.g. for `package.html`) that needs to be
investigated, but this is in `jshell` code and getting out of scope for this
round of work.
-
PR: https://git.openjdk.java.net/jdk/pull/5510
Re: RFR: JDK-8273154: Provide a JavadocTester method for non-overlapping, unordered output matching [v2]
> Please review a moderately simple improvement for `JavadocTester` and a > related new test. > > A new `OutputChecker` class is introduced that mostly supersedes the existing > methods to check the output generated by javadoc and the standard doclet. A > self-imposed restriction is that no existing tests are modified. > > The new class can be used to check files generated by the doclet and the > streams written by the tool. It can be configured to check for ordered output > or not, overlapping output, and complete coverage, and can search for literal > strings and regular expressions. > > There is a corresponding new test which is a non-standard use of > `JavadocTester`, since it is designed to test `JavadocTester` itself, and not > javadoc or the doclet. (Quis custodiet ipsos custodes?) Various methods > are overridden so that the operation of the underlying methods can be checked. > > Although it is a goal to NOT modify the code of any existing tests, it turns > out to be reasonable to adapt some of the existing `check...` methods to use > the new `OutputChecker`. All javadoc tests pass, both locally and on all > standard platforms. Many/most uses of the existing `checkOutput` method > provide "ordered" strings, and are candidates to use the new ordered check. > But enough uses are _not_ ordered, so it is not reasonable to change the > default at this time. It is noted as a TODO to examine the appropriate test > cases, so that we can decide whether to fix those tests and change the > default. Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge with upstream/master - JDK-8273154: Provide a JavadocTester method for non-overlapping, unordered output matching - Changes: https://git.openjdk.java.net/jdk/pull/5743/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5743&range=01 Stats: 814 lines in 2 files changed: 740 ins; 61 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/5743.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5743/head:pull/5743 PR: https://git.openjdk.java.net/jdk/pull/5743
