[PATCH] D55483: Introduce the callback attribute and emit !callback metadata
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 11 13:24:08 PST 2018
jdoerfert added a comment.
Thanks for the feedback.
Once we decided on the style of the annotation, I will implement that and change the tests/documentation accordingly.
================
Comment at: include/clang/Basic/Attr.td:1204
+ VariadicUnsignedArgument<"PayloadIndices">];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
----------------
aaron.ballman wrote:
> jdoerfert wrote:
> > 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 ?).
> > I was thinking that the function notation makes it clear that there is *only one callback per function* allowed right now.
>
> I don't see how that follows. Users may still try writing:
> ```
> __attribute__((callback (1, 3, 4)))
> __attribute__((callback (2, 3, 4)))
> void broker0(void (*cb1)(void *, int), void (*cb2)(void *, int), void *payload, int otherPayload) {
> cb1(payload, otherPayload);
> cb2(payload, otherPayload);
> }
> ```
> and reasonably expect that to work (we should have this as a test case, and probably warn on it).
>
> I'm not strongly opposed to the current way this is exposed to users, just wondering if we can find a better way to surface the feature.
>
> > The last encoding requires us to define a symbol for "unknown argument" (maybe _ or ?).
>
> Ah, I wasn't aware that this was part of the feature, but the documentation you wrote helped to clarify for me. Personal preference is for `?`, but any symbol will do (so long as we aren't hoping users can count commas, e.g., `callback(,,,,frobble,,,foo)`).
> and reasonably expect that to work (we should have this as a test case, and probably warn on it).
We have a test case and we'll spit out an error. (Sema/attr-callback-broken.c line 21 & 22)
> I'm not strongly opposed to the current way this is exposed to users, just wondering if we can find a better way to surface the feature.
I can change it to the inlined style if nobody disagrees:
```
void broker(int foo, void (*callback)(int, int, int, int) __attribute__((callback(foo, ?, bar, ?))), int bar);
```
As I mentioned, I don't have a strong opinion on this but I just don't want to change it back and forth :)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55483/new/
https://reviews.llvm.org/D55483
More information about the llvm-commits
mailing list