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

[Maurizio Cimadamore]

On Wed, 9 Apr 2025 17:12:53 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> 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).
> 
> I suppose what I'm saying is that we don't really care about Switch/SwitchExpression/Block. They have their own way to deal with positions (which javac's backend depends on). Fine.
> 
> What we care about in this PR is that we have valid end position for all the places where a `@SuppressWarnings` annotation can go. I see this as rather orthogonal from cleaning up all the other spurious pos-like fields in the JCTree hierarchy.
> 
> Of course, eventually, we'd like to have a consistent strategy for modelling _all_ end positions -- but it seems to me that it shouldn't be the goal of this PR to get there. Which would suggest that, subjectively, we're better off by leaving existing code be. But again, I defer any final judgment on the matter to @lahodaj

> Practically speaking: @mcimadamore are you OK to proceed with the current approach?

I'm ok. I was mildly surprised to see that the patch touched more than I was expecting (e.g. I did not expect we would touch _existing_ position fields, like the ones in switches). I have no strong preference one way or another -- no matter what we do we'd still have two ways to do very similar things -- and, for that reason, I guess my instinct was to go for something more minimal, given we know we're not done here. But I'm fine either way.

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

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