[PATCH] D148702: [clang] Add Parse and Sema support for RegularKeyword attributes

Wed Apr 19 07:24:47 PDT 2023

rsandifo-arm marked an inline comment as done.
rsandifo-arm added a comment.

Thanks for the reviews!

In D148702#4280392 <https://reviews.llvm.org/D148702#4280392>, @erichkeane wrote:

> So I don't mind the changes in this stack, though this doing a little bit of a 'backdoor' way of getting the arm-streaming attribute in rubs me the wrong way.  I'm not a huge fan that the solution we've got here only solves THIS problem, and doesn't extend to improving the situation with older attributes as well.

Yeah, it looks a bit over-the-top/special-purpose when `__arm_streaming` is the only thing using it.  But we (Arm) have other “semantic attributes” in the pipeline, and this would be useful for them too.  Hopefully other targets will find the same.

We could even go back and retroactively support keywords for some existing Arm semantic attributes.  E.g. maybe we could allow `__aarch64_vector_pcs` alongside `__attribute__((aarch64_vector_pcs))`.  I'd have to see what others think.

But as far as I could tell, there are no existing keyword attributes whose grammar is close enough to standard attributes for the keywords to use the new infrastructure.  E.g. many existing keywords are allowed between qualifiers, whereas standard attributes aren't:

  int (__stdcall a1)(); // OK, but standard attributes aren't allowed in this position
  extern int (*const __stdcall volatile a2) (); // OK, but standard attributes wouldn't be allowed here

I don't think we can retroactively forbid these.  But I don't think it makes sense to allow new attributes like `__arm_streaming` in these positions either.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:5308
       for (const ParsedAttr &AL : DS.getAttributes())
-        Diag(AL.getLoc(), diag::warn_declspec_attribute_ignored)
+        Diag(AL.getLoc(), AL.isRegularKeywordAttribute()
+                              ? diag::err_declspec_keyword_has_no_effect
----------------
erichkeane wrote:
> Is this function (isRegularKeywordAttribute) checking spellings?  If not, it needs to.  
Yeah, this is checking the spelling.  In principle, the series supports attributes that have a RegularKeyword spelling and some other spelling, and in that case, this check would only include attributes that were written using the RegularKeyword spelling.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148702