jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.
In D55483#1326029 <https://reviews.llvm.org/D55483#1326029>, @aaron.ballman
wrote:
> This is missing all of the Sema and SemaCXX tests. Should have tests for
> member functions, variadic functions, incorrect arguments, incorrect
> subjects, etc.
I will write more tests and update this revision.
================
Comment at: include/clang/Basic/Attr.td:1204
+ VariadicUnsignedArgument<"PayloadIndices">];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
----------------
aaron.ballman wrote:
> Should this also apply to Objective-C methods?
>
> Why should the user specify this attribute on the function as opposed to on
> the parameter? e.g.,
> ```
> // Why this:
> __attribute__((callback (1, 2, 3)))
> void* broker0(void* (*callee)(void *), void *payload, int otherPayload) {
> return callee(payload);
> }
>
> // Instead of this:
> void* broker0(void* (*callee)(void *) __attribute__((callback (2, 3))), void
> *payload, int otherPayload) {
> return callee(payload);
> }
>
> // Or this:
> void* broker0(void* (*callee)(void *) __attribute__((callback (payload,
> otherPayload))), void *payload, int otherPayload) {
> return callee(payload);
> }
> ```
> I ask because these "use an index" attributes are really hard for users to
> use in practice. They have to account for 0-vs-1 based indexing, implicit
> this parameters, etc and if we can avoid that, it may be worth the effort.
> Should this also apply to Objective-C methods?
I don't need it to and unless somebody does, I'd say no.
> I ask because these "use an index" attributes are really hard for users to
> use in practice. They have to account for 0-vs-1 based indexing, implicit
> this parameters, etc and if we can avoid that, it may be worth the effort.
I was thinking that the function notation makes it clear that there is *only
one callback per function* allowed right now. I don't expect many manual users
of this feature until we improve the middle-end support, so it is unclear to me
if this requirement needs to be removed as well.
Other than that, some thoughts:
- I do not feel strongly about this.
- The middle requirement seems not much better n the first, we would still need
to deal with index numbers (callbacks without arguments are not really
interesting for now).
- The last encoding requires us to define a symbol for "unknown argument"
(maybe _ or ?).
================
Comment at: include/clang/Basic/Attr.td:1205
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> No new undocumented attributes, please.
Ok, I can write documentation similar to the commit message and the lang-ref
documentation for the callback metadata.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55483/new/
https://reviews.llvm.org/D55483
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
