On Fri, 1 Oct 2021 12:04:53 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
> Unrelated to this PR. Every time I see these lines in JavadocTester, I cannot
> help thinking that we could use more specific APIs:
>
> ```
> public static final String FS = System.getProperty("file.separator");
> public static final String PS = System.getProperty("path.separator");
> public static final String NL = System.getProperty("line.separator");
> public static final String thisRelease =
> System.getProperty("java.specification.version");
> ```
>
> The above lines can be translated into:
>
> ```
> public static final String FS = java.io.File.separator;
> public static final String PS = java.io.File.pathSeparator;
> public static final String NL = System.lineSeparator();
> public static final String thisRelease =
> String.valueOf(Runtime.version().feature());
> ```
>
> I note that neither `PS` nor `thisRelease` seem to be currently used.
I dislike importing `File`, to help catch accidental use of `File` when `Path`
would be better.
You may be right that `PS` and `thisRelease` may not be used, but I'd prefer to
leave them in, just in case.
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 923:
>
>> 921: * Configuration can be done with a series of chained method calls.
>> 922: * Checks can be specified as either literal strings or regular
>> expressions.
>> 923: */
>
> Suggestion:
>
> /**
> * A flexible tool for checking the content of generated files and output
> streams.
> *
> * Configure with a series of chained method calls, specifying target
> strings
> * as either literal strings or regular expressions.
> */
I prefer to keep `checker` instead of `tool`. I'll consider the others.
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 938:
>
>> 936: return new Range(start, end);
>> 937: }
>> 938: boolean overlaps(Range other) {
>
> The `overlaps` method seems unused.
That sounds surprising; I'll check.
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 953:
>
>> 951: * @param file the file
>> 952: */
>> 953: public OutputChecker(String file) {
>
> Given JDK-8274172, consider using java.nio.file.Path instead of String here.
> Using Path for files and String for target strings reduces potential for
> confusion.
I'll investigate, but IIRC there is broad precedent for using `String file` to
simplify the call site, avoiding boilerplate `Path.of`.
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1030:
>
>> 1028: .map(s -> " " + toShortString(s))
>> 1029: .collect(Collectors.joining("\n")));
>> 1030: return this;
>
> Shouldn't we print lines using the platform line separator?
>
> Suggestion:
>
> if (name == null) {
> out.println("Skipping checks for:" + System.lineSeparator()
> + Stream.of(strings)
> .map(s -> " " + toShortString(s))
> .collect(Collectors.joining(System.lineSeparator())));
> return this;
>
> Alternatively, we could println individual lines:
> Suggestion:
>
> if (name == null) {
> out.println("Skipping checks for:");
> for (String s : strings) {
> out.println(" " + toShortString(s));
> }
> return this;
> }
Will consider
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1063:
>
>> 1061: * Checks that there are no duplicate lines in the content.
>> 1062: */
>> 1063: public OutputChecker checkUnique() {
>
> checkUnique seems to be neither used nor tested. As far as I can see, it was
> like that before this PR. Consider removing this method.
I'll check when/why it was added. It must have been added for a reason.
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1076:
>
>> 1074: * if {@code false}, lines that do not match
>> the pattern will be checked
>> 1075: */
>> 1076: public OutputChecker checkUnique(String pattern, boolean
>> select ) {
>
> Suggestion:
>
> public OutputChecker checkUnique(String pattern, boolean select) {
oops
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1079:
>
>> 1077: checking("checkUnique");
>> 1078: Pattern filter = Pattern.compile(pattern);
>> 1079: Matcher m = filter.matcher("");
>
> A reusable matcher; nice.
Yeah, not sure it makes much difference, compared to allocating new ones.
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1167:
>
>> 1165: *
>> 1166: * @param finder a function to find the next occurrence of an
>> item starting at a given position
>> 1167: * @param kind the kind of the item ({@code "text"} or
>> {@code "pattern:} to include in messages
>
> Suggestion:
>
> * @param kind the kind of the item ({@code "text"} or {@code
> "pattern"}) to include in messages
oops
> test/langtools/jdk/javadoc/lib/javadoc/tester/JavadocTester.java line 1227:
>
>> 1225:
>> 1226: private int getLineNumber(int pos) {
>> 1227: Pattern p = Pattern.compile("\\R");
>
> \R is a Unicode linebreak sequence, which includes more than \n and \r\n. Can
> \R give us a spurious line, in particular when analysing HTML output?
It seems better to be more tolerant than pedantic in this case.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5743
