[PATCH] D46805: If some platforms do not support an attribute, we should exclude the platform

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this patch -- be sure to add tests that ensure the attribute is properly prohibited on the expected targets.



================
Comment at: include/clang/Basic/Attr.td:299-300
   list<string> ObjectFormats;
+  // It indicates that a certain attribute is not supported on a specific
+  // platform, turn on support for this attribute by default
+  bit Negated = 0;
----------------
How about: `If set to false, indicates the attribute is supported only on the given target. If set to true, indicates the attribute is supported on all targets except the given target.`


================
Comment at: include/clang/Basic/Attr.td:324-325
 
+// We do not support the alias attribute on Apple platforms,
+// so I define a platform other than the Apple platform.
+def TargetDarwinNegative : TargetArch<["x86", "x86_64"]> {
----------------
This comment is a bit too specific, I'd probably drop the comment entirely as the code is sufficiently clear.


================
Comment at: include/clang/Basic/Attr.td:326
+// so I define a platform other than the Apple platform.
+def TargetDarwinNegative : TargetArch<["x86", "x86_64"]> {
+  let OSes = ["MacOSX"];
----------------
I think we'll want to pattern the names like `TargetNotFoo`.

One other thing is: this isn't really Darwin -- for instance, there's ARM and AArch64 variants, not just x86-based targets.


================
Comment at: include/clang/Basic/Attr.td:327
+def TargetDarwinNegative : TargetArch<["x86", "x86_64"]> {
+  let OSes = ["MacOSX"];
+  let Negated = 1;
----------------
I would expect the OS would be named "Darwin" and not "MacOSX" given the name of the target. Which flavor should be used, or should both be used?


================
Comment at: utils/TableGen/ClangAttrEmitter.cpp:2789
+  // is not supported on this platform
+  if(R->getValueAsBit("Negated"))
+    Test = "!(" + Test + ")";
----------------
The formatting is off here -- there should be a space after the `if`. I would recommend running your patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).


Repository:
  rC Clang

https://reviews.llvm.org/D46805