[PATCH] D121201: [clang] Merge the SourceRange into ParsedAttributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

> I wasn't 100% whether to add the range to ParsedAttributes or even ParsedAttributesView.

I think `ParsedAttributesView` is the correct level for this -- I want to see us tracking the source range for all the parsed attributes (we need the information for diagnostics more often than not), and this ensures we always have access to that information.

> There's still a ParsedAttributesViewWithRange which basically used once in ParseDeclCXX.cpp.

Any idea why we can't get rid of that one? (Maybe it'll become more clear to me as I review more.)



================
Comment at: clang/include/clang/Parse/Parser.h:1583-1591
+  DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributes &attrs,
                                           ParsingDeclSpec *DS = nullptr);
   bool isDeclarationAfterDeclarator();
   bool isStartOfFunctionDefinition(const ParsingDeclarator &Declarator);
-  DeclGroupPtrTy ParseDeclarationOrFunctionDefinition(
-                                                  ParsedAttributesWithRange &attrs,
-                                                  ParsingDeclSpec *DS = nullptr,
-                                                  AccessSpecifier AS = AS_none);
-  DeclGroupPtrTy ParseDeclOrFunctionDefInternal(ParsedAttributesWithRange &attrs,
+  DeclGroupPtrTy
+  ParseDeclarationOrFunctionDefinition(ParsedAttributes &attrs,
+                                       ParsingDeclSpec *DS = nullptr,
----------------



================
Comment at: clang/include/clang/Parse/Parser.h:2071
   StmtResult ParseExprStatement(ParsedStmtContext StmtCtx);
-  StmtResult ParseLabeledStatement(ParsedAttributesWithRange &attrs,
+  StmtResult ParseLabeledStatement(ParsedAttributes &attrs,
                                    ParsedStmtContext StmtCtx);
----------------



================
Comment at: clang/include/clang/Parse/Parser.h:2310
                                   SourceLocation &DeclEnd,
-                                  ParsedAttributesWithRange &attrs,
+                                  ParsedAttributes &attrs,
                                   SourceLocation *DeclSpecStart = nullptr);
----------------



================
Comment at: clang/include/clang/Parse/Parser.h:2314
   ParseSimpleDeclaration(DeclaratorContext Context, SourceLocation &DeclEnd,
-                         ParsedAttributesWithRange &attrs, bool RequireSemi,
+                         ParsedAttributes &attrs, bool RequireSemi,
                          ForRangeInit *FRI = nullptr,
----------------
I'll stop commenting on these -- if you see a function signature where the parsed attribute parameter identifier uses the wrong coding style *and* the rest of the parameters already use the right style, might as well hit those identifiers. No need to fix all naming issues with the touched functions (unless you feel like doing an NFC commit to change them).


================
Comment at: clang/lib/Parse/ParseDecl.cpp:500
 /// a C++11 attribute in "gnu" namespace.
-void Parser::ParseGNUAttributeArgs(IdentifierInfo *AttrName,
-                                   SourceLocation AttrNameLoc,
-                                   ParsedAttributes &Attrs,
-                                   SourceLocation *EndLoc,
-                                   IdentifierInfo *ScopeName,
-                                   SourceLocation ScopeLoc,
-                                   ParsedAttr::Syntax Syntax,
-                                   Declarator *D) {
+void Parser::ParseGNUAttributeArgs(
+    IdentifierInfo *AttrName, SourceLocation AttrNameLoc,
----------------
It looks like some unrelated formatting changes snuck in? I'm not opposed, but it's a bit easier to review if this was an NFC commit (before or after landing this patch) because it makes for a smaller diff here. (I noticed this in a few spots, only commenting about it here.)


================
Comment at: clang/test/SemaOpenCL/address-spaces.cl:261
   typedef __private int private_int_t;
-  __private __attribute__((opencl_global)) int var1;   // expected-error {{multiple address spaces specified for type}} \
+  __attribute__((opencl_global)) __private int var1;   // expected-error {{multiple address spaces specified for type}} \
                                                        // expected-error {{function scope variable cannot be declared in global address space}}
----------------
tbaeder wrote:
> This is a peculiar ordering problem...
I think we need to ensure that we still diagnose both ways, otherwise we'll start to accept invalid code: https://godbolt.org/z/h7473Ehc5

Or does the behavior change here in other ways?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121201/new/

https://reviews.llvm.org/D121201