[PATCH] D43248: [Attr] Fix printing of parameter indices in attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:182
+  // it would always be false.
+  string DisallowImplicitThisParamName = disallowImplicitThisParamName;
+}
----------------
Is there much benefit to forcing the attribute author to pick a name for this? It seems like this is more of a Boolean value: either implicit this is allowed or not. It would be really nice if we could hide these mechanics as implementation details so that the user only needs to write `VariadicParamIndexArgument<"name", DisallowImplicitThis>` (or something similar) when defining the attribute, and ideally not have to do any extra work in SemaDeclAttr.cpp by taking care of it in ClangAttrEmitter.cpp if possible.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:355-357
   }
+  else if (DisallowImplicitThisParam)
+    *DisallowImplicitThisParam = false;
----------------
Formatting is off here -- the `else if` should go up a line.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:4612
+                                           TypeTagIdx, false,
+                                           &DisallowImplicitThisParam))
     return;
----------------
This value cannot be different from the previously-read value, correct? Might want to assert that.


================
Comment at: test/Sema/attr-print.cpp:1
+// RUN: %clang_cc1 %s -ast-print | FileCheck %s
+
----------------
-ast-print tests generally live in Misc rather than Sema -- can you move this test over to that directory?


================
Comment at: test/Sema/attr-print.cpp:33
+  // CHECK: void ownr(int) __attribute__((ownership_returns(foo, 2)));
+};
----------------
Can you add some tests for `__attribute__((format))` and `__attribute__((format_arg))` to demonstrate that their pretty-printed argument values are also sensible?


https://reviews.llvm.org/D43248