aaron.ballman 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: > > 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)`). ================ Comment at: include/clang/Basic/AttrDocs.td:3711 +The ``callback`` attribute specifies that the annotated function may invoke the +specified callback callee zero or more times. The callback callee, as well as +the passed arguments, are identified by their parameter position (starting with ---------------- I'd remove `callee` here in both places. ================ Comment at: include/clang/Basic/AttrDocs.td:3714 +1!) in the annotated function. The first position identifies the callback callee, +the following indices the forwarded arguments. The callback callee is required +to be callable with the number, and order, of the specified arguments. To ---------------- and the following indicies are the forwarded arguments. ================ Comment at: include/clang/Basic/AttrDocs.td:3716 +to be callable with the number, and order, of the specified arguments. To +represent unknown arguments a zero shall be used. + ---------------- The index '0' is used to represent a callback callee argument which is not present in the declared parameter list. ================ Comment at: include/clang/Basic/AttrDocs.td:3718 + +The ``callback`` attributes, which are directly translated to ``callback`` +metadata <http://llvm.org/docs/LangRef.html#callback-metadata>, allow to make ---------------- attributes -> attribute ================ Comment at: include/clang/Basic/AttrDocs.td:3719 +The ``callback`` attributes, which are directly translated to ``callback`` +metadata <http://llvm.org/docs/LangRef.html#callback-metadata>, allow to make +the connection between the call to the annotated function and the callback ---------------- , allow to make the connection -> , make the connection ================ Comment at: include/clang/Basic/AttrDocs.td:3721 +the connection between the call to the annotated function and the callback +callee. This can enable interprocedural optimizations which were otherwise +impossible. If a function parameter is mentioned in the ``callback`` ---------------- Switch to consistently using a single space after the period (as was done in the previous paragraph). ================ Comment at: include/clang/Basic/AttrDocs.td:3723 +impossible. If a function parameter is mentioned in the ``callback`` +attribute, through its position, it is undefinied if that parameter is used for +anything else than then actual callback. Thus, inspected, captured, or modified ---------------- undefinied -> undefined ================ Comment at: include/clang/Basic/AttrDocs.td:3724 +attribute, through its position, it is undefinied if that parameter is used for +anything else than then actual callback. Thus, inspected, captured, or modified +parameters shall may be mentioned in the ``callback`` metadata. ---------------- else than then actual callback -> anything other than the actual callback. Thus, inspected -> Inspected, ================ Comment at: include/clang/Basic/AttrDocs.td:3725 +anything else than then actual callback. Thus, inspected, captured, or modified +parameters shall may be mentioned in the ``callback`` metadata. + ---------------- shall may be mentioned -> are listed ================ Comment at: include/clang/Basic/AttrDocs.td:3730-3731 +(`start_routine`) is called zero or more times by the `pthread_create` function, +and that the fourth parameter (`arg`) is passed along. Note that this case is +actually automatically recognized. + ---------------- You should mention the circumstances under which we automatically recognize a callback so that users know when they're required to write the annotation vs allowing the compiler to infer it. 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