This is an archive of the discontinued LLVM Phabricator instance.

Include leading attributes in DeclStmt's SourceRange
ClosedPublic

Authored by sberg on Oct 7 2019, 8:22 AM.

Details

Summary

For a DeclStmt like

[[maybe_unused]] int i;

getBeginLoc() returned the start of int instead of the start of [[ (see http://lists.llvm.org/pipermail/cfe-dev/2019-September/063434.html "[cfe-dev] Poor DeclStmt source range with leading [[attributes]]?").

Diff Detail

Event Timeline

sberg created this revision.Oct 7 2019, 8:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 8:22 AM

I think this seems like reasonable behavior (for instance, we include the location of a storage class specifier already), but I am curious if @rsmith agrees.

Is there a good way to write a test for this?

Yes, you can put an AST dumping test into the test\AST directory to check that you get expected source locations.

clang/lib/Parse/ParseStmt.cpp
224

Indentation looks incorrect here.

I think this seems like reasonable behavior (for instance, we include the location of a storage class specifier already), but I am curious if @rsmith agrees.

Yes, I do agree. Though please be careful about the case where the first attribute occurs after the first decl-specifier (int __attribute__((...)) x;).

sberg updated this revision to Diff 224303.Oct 10 2019, 3:56 AM
sberg edited the summary of this revision. (Show Details)
sberg marked an inline comment as done.
aaron.ballman added inline comments.Oct 13 2019, 12:17 PM
clang/test/AST/sourceranges.cpp
147 ↗(On Diff #224303)

I think these CHECK lines should just be a normal check instead of a C++17 check.

sberg updated this revision to Diff 224823.Oct 14 2019, 2:27 AM
sberg marked an inline comment as done.
aaron.ballman accepted this revision.Oct 16 2019, 8:08 AM

LGTM, assuming the requested test changes are as successful as I expect they will be (doesn't need additional review if the tests pass).

clang/test/AST/sourceranges.cpp
155 ↗(On Diff #224823)

Why is this guarded on C++17? [[maybe_unused]] is supported in every language mode used by this test (it generates a warning, but we're FileCheck'ing so that's fine -- but you could use [[deprecated("")]] instead if you want).

Additionally, I'd appreciate one more test case testing a __declspec attribute as well, to make sure we're handling that properly as well.

This revision is now accepted and ready to land.Oct 16 2019, 8:08 AM
This revision was automatically updated to reflect the committed changes.
sberg marked 2 inline comments as done.Oct 17 2019, 5:41 AM
sberg added inline comments.
clang/test/AST/sourceranges.cpp
155 ↗(On Diff #224823)

Oh, didn't know that [[maybe_unused]] would also be accepted in pre-C++17 modes (I was somehow under the false impression that this file would be processed as -std=c++11 by default, where [[deprecated("")]] wouldn't have worked either).
Testing both attribute and declspec at both the start and in the middle of the stmt now. (One caveat with declspec is that its location resolves to <builtin>, though.)