On Mon, 26 Jan 2026 11:17:25 GMT, Maurizio Cimadamore <[email protected]>
wrote:
>> Liam Miller-Cushon 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 seven
>> additional commits since the last revision:
>>
>> - Merge remote-tracking branch 'origin/master' into JDK-8372948
>> - Merge remote-tracking branch 'origin/master' into JDK-8372948
>> - Fix DiagnosticGetEndPosition on windows
>> - Debug DiagnosticGetEndPosition.java on windows
>> - Update assertion for
>> test/langtools/tools/javac/diags/DiagnosticGetEndPosition.java
>> - Merge remote-tracking branch 'origin/master' into JDK-8372948
>> - 8372948: Store end positions directly in JCTree
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/ParserFactory.java
> line 92:
>
>> 90: }
>> 91:
>> 92: public JavacParser newParser(CharSequence input, boolean
>> keepDocComments, boolean keepLineMap) {
>
> Unrelated to this PR, but I wonder how much value there is in having two
> different factories here where the only difference is the omission of a
> trailing boolean param
Another thing about these overloads I just realized is that before and after
this change there is an overload that takes three boolean parameters, but the
meaning of them has changed: `keepDocComments, keepEndPos, keepLineMap` vs
`keepDocComments, keepLineMap, parseModuleInfo`.
I'd forgotten to update some call sites because they still compiled. I have
fixed them now, and it didn't matter in those cases because they were passing
`false` for all of the options, but it does show these overloads could be
bug-prone.
I'd be happy to try to improve this as a follow up if you think that'd be
worthwhile. Having a single `newParser` overload could be safer because it
forces callers to update if the signature changes again. Another possibility
might be something like
`parserFactory.newParser(input).keepDocComments(true).keepLineMap(true).parse()`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28610#discussion_r2748297774
