Include leading attributes in DeclStmt's SourceRange

[sberg]

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]]?").

[aaron.ballman]

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.

[rsmith]

In D68581#1701787, @aaron.ballman wrote:

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;).

[aaron.ballman]

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.

[aaron.ballman]

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.

[sberg]

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.)

clang/lib/Parse/ParseStmt.cpp

if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
       if (GNUAttributeLoc.isValid()) {
         DeclStart = GNUAttributeLoc;
         Decl = ParseDeclaration(DeclaratorContext::BlockContext, DeclEnd, Attrs,
                                 &GNUAttributeLoc);
       } else {
         Decl =
             ParseDeclaration(DeclaratorContext::BlockContext, DeclEnd, Attrs);
       }
+      if (Attrs.Range.getBegin().isValid())
+          DeclStart = Attrs.Range.getBegin();
       return Actions.ActOnDeclStmt(Decl, DeclStart, DeclEnd);
     }
 
     if (Tok.is(tok::r_brace)) {
       Diag(Tok, diag::err_expected_statement);
       return StmtError();
     }