RFR: 8350212: Track source end positions of declarations that support @SuppressWarnings [v3]

[Archie Cobbs]

On Tue, 8 Apr 2025 14:56:41 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> Archie Cobbs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add missing variable decl end position.
>
> The new calls to `storeEnd` seem to partly duplicate existing `to`/`toP` calls. That feels a bit unfortunate - as we may be adding new places that create AST node for declarations, and one sometimes forgets to do `to`/`toP` - and forgetting `storeEnd` might lead to weird problems.
> 
> I tried to re-write this part by putting the the set logic directly into the end pos tables here:
> https://github.com/archiecobbs/jdk/compare/JDK-8350212...lahodaj:jdk:JDK-8350212?expand=1
> 
> Notes:
> - I believe the fact that implictly declared classes don't have an end position was intentional at that time. I don't have an issue with changing that, just pointing it out.
> - for blocks, `endpos` is actually pointing at the start of the closing brace, not the end (I think, at least). So, I've renamed the field, but kept the semantics.
> 
> What do you think?

> I'll leave this to @lahodaj . In my opinion the removal of the `endpos` field in `JCSwitch` adds more code than it removes -- because of that `TreeInfo.endPos` needs a new `EndPosTable` parameter, which means `Flow`/`Lower`/`TransPatterns` needs additional logic to set the end pos table. It's not a complex rewriting, but given that we kind of know we're going to put our hands in here again, I wonder if there could be some merit in being more minimal/pragmatic.

Well the point of the latest refactoring was to try to remove the storage of duplicate end positions and move to a single unified way of accessing ending positions via `EndPosTable` (the `bracePos` fields are not duplicate, because they point to different positions).

Your suggestion now is to perhaps go back in the reverse direction. That's not totally crazy, but I feel like now I'm getting mixed messages.

FWIW my personal opinion is that the `EndPosTable` is readily accessible already, so it's OK to require it, and this makes the code more consistent without special exceptions. Of course it also comes with a small performance penalty - but then I remember [this](https://wiki.c2.com/?PrematureOptimization).

-------------

PR Comment: https://git.openjdk.org/jdk/pull/23669#issuecomment-2790032282