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

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.


================
Comment at: include/clang/Basic/Attr.td:182
+  // it would always be false.
+  string DisallowImplicitThisParamName = disallowImplicitThisParamName;
+}
----------------
aaron.ballman wrote:
> 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.
Thanks for the review.  I'll work on that.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:355-357
   }
+  else if (DisallowImplicitThisParam)
+    *DisallowImplicitThisParam = false;
----------------
aaron.ballman wrote:
> Formatting is off here -- the `else if` should go up a line.
I don't follow.  It looks right to me. 


================
Comment at: lib/Sema/SemaDeclAttr.cpp:4612
+                                           TypeTagIdx, false,
+                                           &DisallowImplicitThisParam))
     return;
----------------
aaron.ballman wrote:
> This value cannot be different from the previously-read value, correct? Might want to assert that.
Right.  Hopefully this will go away after I apply your initial suggestion.


================
Comment at: test/Sema/attr-print.cpp:1
+// RUN: %clang_cc1 %s -ast-print | FileCheck %s
+
----------------
aaron.ballman wrote:
> -ast-print tests generally live in Misc rather than Sema -- can you move this test over to that directory?
Sure.  Should the existing test/Sema/attr-print.c move too?


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


https://reviews.llvm.org/D43248