[PATCH] D148700: [clang] Add support for “regular” keyword attributes

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added a subscriber: sdesmalen.
rsandifo-arm added a comment.

Thanks for the review.



================
Comment at: clang/include/clang/Basic/Attr.td:2427-2430
+def ArmStreaming : TypeAttr, TargetSpecificAttr<TargetAArch64> {
+  let Spellings = [RegularKeyword<"__arm_streaming">];
+  let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> I'd feel more comfortable switching an existing attribute over to use this new functionality instead of introducing a new attribute at the same time. (Also, we ask that there be no new undocumented attributes unless there's a Very Good Reason to leave it undocumented.)
Could you suggest an attribute that would be suitable?  The problem is that, as far as I know, no existing keyword has parse rules that exactly match the rules for standard attributes.  (That, and the fact that different keywords have different rules, was one of the main motivations for the patch).  So I don't think it would be possible to adopt the new scheme for existing attributes without breaking compatiblity (both ways: some things would become valid that weren't before, and some things that were valid before would become invalid).

E.g.:

==== __declspec

I don't think anyone was suggesting we try to convert this :-) but for completeness:

```
int __declspec(align(16)) a1; // OK, but standard attribute rules would say this appertains to the type
int a2 __declspec();          // Not valid, unlike for standard decl attributes
```

==== __forceinline

```
int __forceinline a1() { return 1; } // OK, but standard attribute rules would say this appertains to the type
int a2 __forceinline() { return 1; } // Not valid, but would be for standard decl attributes
```

==== Calling convention keywords

These are generally DeclOrType attributes, with attributes sliding from the decl to the type where necessary.

```
__stdcall int a1();   // OK, but appertains to the decl and relies on sliding behaviour
int a2 __stdcall();   // Not valid, but is another place to put standard decl attributes
int a3() __stdcall;   // Not valid, but is another place to put standard type attributes
int (__stdcall a4)(); // OK, but standard attributes aren't allowed in this position
extern int (*const __stdcall volatile a5) (); // OK, but standard attributes wouldn't be allowed here
```

==== Nullability keywords

Like the calling-convention keywords, the nullability keywords appertain to types but are allowed in some (but not all) positions that would normally appertain to decls:

```
using ptr = int *;
_Nullable ptr a1; // OK, but appertains to the decl and relies on sliding behaviour
ptr a2 _Nullable; // Not valid, but is another place to put standard decl attributes
extern int *const _Nullable volatile a3; // OK, but a standard attribute wouldn't be allowed here
```

The same distinction applies to Microsoft pointer attributes.

On the documentation side: I admit this is one of the awkward things about using a new keyword to prove the system.  The series doesn't actually implement `__arm_streaming` (@sdesmalen has patches for that, and other SME stuff).  So it didn't seem appropriate to document the attribute until it was actually implemented.  But the implementation, when it comes, will/should have documentation.


================
Comment at: clang/lib/AST/TypePrinter.cpp:1724-1727
+  if (T->getAttrKind() == attr::ArmStreaming) {
+    OS << "__arm_streaming";
+    return;
+  }
----------------
aaron.ballman wrote:
> This seems like something that tablegen should automatically handle more generally for these attributes.
I wondered about trying to use tablegen for TypePrinter::printAttributedAfter, but decided against it. 
 RegularKeyword is really a spelling-level classification rather than an attribute-level classification, and in general, an attribute could have both GNU and RegularKeyword spellings. In contrast, printAttributedAfter is only given the attribute kind and the type that results from applying the attribute. AFAIK, it doesn't have access to the original attribute spelling. This means that some attribute-specific or type-specific knowledge might be needed to print the attribute in the best way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148700