On Fri, 30 Jan 2026 22:55:00 GMT, Liam Miller-Cushon <[email protected]> wrote:
>> 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()`.
For me - I think there was a time where the idea was that some of the internal
APIs (like to parser factories) would be evolved somewhat "compatibly". I don't
think that's necessary anymore (and compatible evolution is not being done
anyway), esp. for things like parser factories. So, for me: we can clear the
factories and only keep one, that's OK and will improve maintainability.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28610#discussion_r2758275777
