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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Just NFC nits from me with coding style stuff, so this LGTM! Feel free to fix the style nits when you land or land this as-is and do an NFC commit to fix them.



================
Comment at: clang/include/clang/Parse/Parser.h:2770
   }
-  void ParseCXX11Attributes(ParsedAttributesWithRange &attrs);
+  void ParseCXX11Attributes(ParsedAttributes &attrs);
   /// Parses a C++11 (or C2x)-style attribute argument list. Returns true
----------------



================
Comment at: clang/include/clang/Parse/Parser.h:3008
       DeclaratorContext Context, const ParsedTemplateInfo &TemplateInfo,
-      SourceLocation &DeclEnd, ParsedAttributesWithRange &attrs);
+      SourceLocation &DeclEnd, ParsedAttributes &attrs);
   Decl *ParseUsingDirective(DeclaratorContext Context,
----------------



================
Comment at: clang/include/clang/Sema/DeclSpec.h:2516-2521
+  void takeAttributes(ParsedAttributes &attrs) {
     Attrs.takeAllFrom(attrs);
 
-    if (!lastLoc.isInvalid())
-      SetRangeEnd(lastLoc);
+    if (attrs.Range.getEnd().isValid())
+      SetRangeEnd(attrs.Range.getEnd());
   }
----------------



================
Comment at: clang/lib/Parse/ParseDecl.cpp:1424
+    IdentifierInfo &ObjCBridgeRelated, SourceLocation ObjCBridgeRelatedLoc,
+    ParsedAttributes &attrs, SourceLocation *endLoc, IdentifierInfo *ScopeName,
+    SourceLocation ScopeLoc, ParsedAttr::Syntax Syntax) {
----------------



================
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,
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > 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.)
> I've tried reverting some of them, but `git clang-format` adds them back every time...
Huh, that's very odd!


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:474
+    DeclaratorContext Context, const ParsedTemplateInfo &TemplateInfo,
+    SourceLocation &DeclEnd, ParsedAttributes &attrs) {
   assert(Tok.is(tok::kw_using) && "Not using token");
----------------



================
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:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > 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?
> > can you debug this to see what the problem is?  I wouldn't expect the ordering to matter here.
> I investigated this a bit, which is why I know it's an ordering problem. Let's see...
> 
> The "multiple address spaces specified for type" error is emitted both before and after this patch, so this is about the second warning about function scope variables. That warning is emitted in `Sema::CheckVariableDeclarationType()` if the address space of the type is `LangAS::opencl_global`.
> 
> In `HandleAddressSpaceTypeAttribute()` in `SemaType.cpp`, the "multiple address spaces specified for type" error is emitted, which marks the second attribute invalid and ignores it. So if the `__attribute__((opencl_global))` is handled second, it's ignored and won't reach `CheckVariableDeclarationType()`, where the second error is emitted.
Ah, thank you for this -- so both situations are diagnosed correctly, it's just a matter that cascading error behavior changed. I'm fine with that.


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

https://reviews.llvm.org/D121201