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