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

163

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

171

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

171

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();
     }
 

clang/test/AST/sourceranges.cpp

 struct map {
   using T = pair<K, V>;
   map(initializer_list<T> i, const string &s = string()) {}
   ~map() { abort(); }
 };
 
 }; // namespace std
 
+// CHECK: NamespaceDecl {{.*}} attributed_decl
+namespace attributed_decl {
+  void f() {
+    // CHECK: DeclStmt {{.*}} <line:[[@LINE+1]]:5, col:28>
+    [[maybe_unused]] int i1;
+    // CHECK: DeclStmt {{.*}} <line:[[@LINE+1]]:5, col:35>
+    __attribute__((unused)) int i2;
+    // CHECK: DeclStmt {{.*}} <line:[[@LINE+1]]:5, col:35>
+    int __attribute__((unused)) i3;
+    // CHECK: DeclStmt {{.*}} <<built-in>:{{.*}}, {{.*}}:[[@LINE+1]]:40>
+    __declspec(dllexport) extern int i4;
+    // CHECK: DeclStmt {{.*}} <line:[[@LINE+1]]:5, col:40>
+    extern int __declspec(dllexport) i5;
+  }
+}
+
 #if __cplusplus >= 201703L
 // CHECK-1Z: FunctionDecl {{.*}} construct_with_init_list
 std::map<int, int> construct_with_init_list() {
   // CHECK-1Z-NEXT: CompoundStmt
   // CHECK-1Z-NEXT: ReturnStmt {{.*}} <line:[[@LINE+5]]:3, col:35
   // CHECK-1Z-NEXT: ExprWithCleanups {{.*}} <col:10, col:35
   // CHECK-1Z-NEXT: CXXBindTemporaryExpr {{.*}} <col:10, col:35
   // CHECK-1Z-NEXT: CXXTemporaryObjectExpr {{.*}} <col:10, col:35

 // CHECK-1Z: NamespaceDecl {{.*}} new_init
 namespace new_init {
   void A() {
     // CHECK-1Z: CXXNewExpr {{.*}} <line:[[@LINE+2]]:5, col:14
     // CHECK-1Z-NEXT: InitListExpr {{.*}} <col:12, col:14
     new int{0};
   }
 }
 #endif