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?

Hi @lahodaj,

Maurizio's comments got me looking into this more and I came to the same realizations... the `storeEnd()` calls are just doing the same thing as `to()` and `toP()` so they can be cleaned up. I also like his idea of making them fluent (and btw `attach()` can also be fluent).

Finally, I agree that his idea of replacing `EmptyEndPosTable` with `MinimalEndPosTable` and not adding the duplicate, one-off `endPos` fields is also cleaner.

So I'm working on a refactoring that will include the above ideas. I'll have an update soon.

Thanks for clarifying the difference between `endPos` and `bracePos` - it's been causing me confusing test failures. I think that's the one "one-off" field we'll probably need to keep.

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

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