jdoerfert marked 3 inline comments as done.
jdoerfert added inline comments.
================
Comment at: include/clang/Basic/Attr.td:1204
+ VariadicUnsignedArgument<"PayloadIndices">];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
----------------
jdoerfert wrote:
> 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 :)
>
You can now use parameter names or indices in the callback attribute. The
special identifiers "__" (double underscore) and "__this" can be used to
specify an unknown (-1 in the index notation) and the implicit "this" (0 in the
index notation) argument.
================
Comment at: test/CodeGen/callback_qsort_r.c:1
+// RUN: %clang %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -D_GNU_SOURCE %s -S -c -emit-llvm -o - -O1 | FileCheck %s
--check-prefix=IR
----------------
arichardson wrote:
> This should use a linux triple otherwise the qsort_r declaration is wrong.
> Ideally it should also handle macos+FreeBSD with the inverted argument order.
I postponed qsort_r until we have another similar function or somebody want to
implement the necessary functionality.
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
