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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
