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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder marked 3 inline comments as done.
tbaeder added inline comments.


================
Comment at: clang/lib/Parse/ParseCXXInlineMethods.cpp:745
 
-      ParseGNUAttributeArgs(&LA.AttrName, LA.AttrNameLoc, Attrs, &endLoc,
+      ParseGNUAttributeArgs(&LA.AttrName, LA.AttrNameLoc, Attrs, nullptr,
                             nullptr, SourceLocation(), ParsedAttr::AS_GNU,
----------------
erichkeane wrote:
> This change is concerning, shouldn't ParseGNUAttributeArgs just be losing its 'EndLoc' pointer instead?
IIRC this is just a cleanup - as you can see, the local `endLoc` is unused apart from being passed to those two function calls, which can handle `nullptr` just fine.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3207
+  if (Tok.is(tok::kw___attribute)) {
+    ParsedAttributes Attrs(AttrFactory);
     MaybeParseGNUAttributes(Attrs);
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > This seems like a particularly strange change, What is the reasoning for this?  Is it just that the Attrs are unused?
> The comment above is that we parse and discard any trailing attributes, so this change is scoping the `ParsedAttributes` object more tightly to the only scope it's needed, but otherwise not changing behavior.
Yup, both right. The change is not strictly necessary but makes sense IMO.


================
Comment at: clang/lib/Sema/SemaType.cpp:8129
+
+  state.setParsedNoDeref(false);
+  if (attrs.empty())
----------------
erichkeane wrote:
> What is the justification for this set of changes?  Is it simply to 'early exit' rather than make the unnecessary copy?
It avoids a copy and made debugging the opencl ordering problem a bit easier as the loop below is not reached for empty attribute lists. I can leave it out of this patch if you want.


================
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}}
----------------
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.


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

https://reviews.llvm.org/D121201