This is an archive of the discontinued LLVM Phabricator instance.

[clang] Stop dragging a EndLoc around when parsing attributes
ClosedPublic

Authored by tbaeder on Mar 3 2022, 3:21 AM.

Details

Summary

I noticed that this is basically always unused. And if it is needed, the end of the attribute range can be used.

Diff Detail

Event Timeline

tbaeder created this revision.Mar 3 2022, 3:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 3:21 AM
tbaeder requested review of this revision.Mar 3 2022, 3:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 3:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 412714.Mar 3 2022, 7:27 AM

Thank you for working on this! We've (very) slowly been working towards all of the attribute parsing functions taking a ParsedAttributesWithRange so that we don't lose this information in the AST, and this is a good step in the right direction towards that.

clang/lib/Parse/ParseDecl.cpp
820–821

This is a case where we're regressing functionality -- we used to track the actual end location here, but because this doesn't take a ParsedAttributesWithRange, the end location is lost. Can we rework this to take a ParsedAttributesWithRange instead?

clang/lib/Parse/ParseDeclCXX.cpp
4661–4662

Heh, same concern here as above.

clang/lib/Parse/ParseStmt.cpp
1121–1122

It seems like this could be re-flowed to 80-col?

tbaeder marked an inline comment as done.Mar 4 2022, 6:15 AM

Thank you for working on this! We've (very) slowly been working towards all of the attribute parsing functions taking a ParsedAttributesWithRange so that we don't lose this information in the AST, and this is a good step in the right direction towards that.

Yes, that's what I'm working on. There is a very peculiar test failure in clang/test/SemaOpenCL/address-spaces.cl however, so I can't switch everything over to ParsedAttributesWithRange (and do a rename) at once.

clang/lib/Parse/ParseDecl.cpp
820–821

Pretty sure yes. I already have that change locally anyway, I'll add it to this patch and run the tests.

clang/lib/Parse/ParseDeclCXX.cpp
4661–4662

Also same as above :)

tbaeder updated this revision to Diff 412994.Mar 4 2022, 6:34 AM
tbaeder marked an inline comment as done.
aaron.ballman accepted this revision.Mar 4 2022, 11:00 AM

LGTM aside from two formatting nits, thank you for the cleanup!

clang/lib/Parse/ParsePragma.cpp
1373–1374

Probably worth reformatting this before you land.

This revision is now accepted and ready to land.Mar 4 2022, 11:00 AM
tbaeder updated this revision to Diff 413276.Mar 5 2022, 11:36 PM
This revision was landed with ongoing or failed builds.Mar 6 2022, 11:17 PM
This revision was automatically updated to reflect the committed changes.