aaron.ballman added inline comments.
================
Comment at: include/clang/Basic/Attr.td:1204
+ VariadicUnsignedArgument<"PayloadIndices">];
+ let Subjects = SubjectList<[Function]>;
+ let Documentation = [Undocumented];
----------------
jdoerfert wrote:
> 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.
Nice! I like that approach.
================
Comment at: include/clang/Basic/AttrDocs.td:3778
+The callback callee is required to be callable with the number, and order, of
+the specified arguments. The index '0', or the identifier "this", is used to
+represent an implicit "this" pointer in class methods. If there is no implicit
----------------
I'd use backticks around `0` and `this` rather than single and double quotes,
to make it clear that these are syntax and not prose.
================
Comment at: include/clang/Basic/AttrDocs.td:3782-3783
+represents an unknown callback callee argument. This can be a value which is
+not present in the declared parameter list, or one that is but potentially
+inspected, captured, or modified. Parameter names and indices can be mixed in
+the callback attribute.
----------------
Grammatical issue here -- I think it should probably say "or one that is, but
is potentially..."
================
Comment at: include/clang/Basic/AttrDocs.td:3786
+
+The ``callback`` attribute, which are directly translated to ``callback``
+metadata <http://llvm.org/docs/LangRef.html#callback-metadata>, make the
----------------
are -> is
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2578
+def err_callback_attribute_argument_unknown : Error<
+ "'callback' argument '%0' is not a known function parameter">;
+def err_callback_attribute_argument_index_oob : Error<
----------------
I think this should say `'callback' attribute` (here and in the other
diagnostics); WDYT?
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2579-2580
+ "'callback' argument '%0' is not a known function parameter">;
+def err_callback_attribute_argument_index_oob : Error<
+ "'callback' argument at position %0 is out-of-bounds">;
+def err_callback_incomplete_function_type : Error<
----------------
This is not needed, see `err_attribute_argument_out_of_bounds`.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2586
+def err_callback_callee_is_variadic : Error<
+ "'callback' callee shall not be variadic">;
+def err_callback_implicit_this_not_available : Error<
----------------
I'd reword this to use "may not" instead of "shall not".
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2589-2592
+def err_callback_too_few_arguments : Error<
+ "'callback' attribute specifies too few arguments">;
+def err_callback_too_many_arguments : Error<
+ "'callback' attribute specifies too many arguments">;
----------------
These are not needed, see `err_attribute_too_many_arguments` and
`err_attribute_too_few_arguments` (or `err_attribute_wrong_number_arguments`).
================
Comment at: lib/Sema/SemaDecl.cpp:13587-13588
+ SmallVector<int, 4> Encoding;
+ if (Context.BuiltinInfo.performsCallback(BuiltinID, Encoding)) {
+ if (!FD->hasAttr<CallbackAttr>())
+ FD->addAttr(CallbackAttr::CreateImplicit(
----------------
You can combine these predicates into one `if` statement.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3481
+ llvm::StringMap<int> NameIdxMapping;
+ NameIdxMapping["__"] = -1;
+
----------------
This doesn't match the documentation -- I assume you switched from `?` to `__`
because `__` at least parses as a valid identifier, whereas `?` would require
extra parsing support? If so, that's fine by me.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3483
+
+ NameIdxMapping["__this"] = 0;
+
----------------
This doesn't match the documentation either, but I'm less clear on why the
double underscores are used.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3492
+ SmallVector<int, 8> EncodingIndices;
+ for (unsigned u = 0, e = AL.getNumArgs(); u < e; u++) {
+
----------------
Identifiers don't match the usual naming conventions. Prefer `++U` as well.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3493
+ for (unsigned u = 0, e = AL.getNumArgs(); u < e; u++) {
+
+ SourceRange SR;
----------------
Spurious newline
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3502
+ S.Diag(AL.getLoc(), diag::err_callback_attribute_argument_unknown)
+ << IdLoc->Ident->getName() << IdLoc->Loc;
+ return;
----------------
You can pass in `IdLoc->Ident` and the diagnostic engine should properly print
and quote it for you.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3508
+ ArgIdx = It->second;
+
+ } else if (AL.isArgExpr(u)) {
----------------
Spurious newline.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:3528
+ SR = IdxExpr->getSourceRange();
+
+ } else {
----------------
Newline
================
Comment at: test/CodeGen/callback_annotated.c:18-19
+// RUN1-DAG: declare !callback ![[cid2:[0-9]+]] i8* @broker2
+__attribute__((callback (callee)))
+void* broker2(void (*callee)(void));
+
----------------
Can you add a test like this:
```
void* broker2(void *(*callee)(void));
void* test(void) {
return broker2(test);
}
__attribute__((callback (callee)))
void* broker2(void *(*callee)(void));
```
Basically, I'm trying to see whether we properly merge the declarations and
note that the call to `broker2()` within `test()` really does have the
`callback` attribute.
================
Comment at: test/Sema/attr-callback-broken.c:5
+
+__attribute__((callback(1, 1))) void too_many_args_1(void
(*callback)(void)) {} // expected-error {{'callback' attribute specifies too
many arguments}}
+__attribute__((callback(1, -1))) void too_many_args_2(double
(*callback)(void)); // expected-error {{'callback' attribute specifies too
many arguments}}
----------------
Please run the tests through clang-format.
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
