aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:3445
+ let Spellings = [Clang<"acquire_handle">];
+ let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+ let Documentation = [AcquireHandleDocs];
----------------
xazax.hun wrote:
> xazax.hun wrote:
> > aaron.ballman wrote:
> > > xazax.hun wrote:
> > > > aaron.ballman wrote:
> > > > > What about function-like interfaces such as lambdas, blocks, or other
> > > > > callable objects that are not a `FunctionDecl`?
> > > > Good point!
> > > This get you partway there -- `FunctionLike` allows functions and
> > > function pointers, but I don't think it allows lambdas or blocks. You
> > > should add some test cases for all of these (including function pointers)
> > > to verify you're able to mark the entities you want and get the behavior
> > > you're after.
> > >
> > > For instance, on function pointers and lambdas, this may impact the
> > > function *type* which is a good design question to be thinking about.
> > > What should happen with code like:
> > > ```
> > > [[clang::acquire_handle]] void int open(const char *);
> > > int (*fp)(const char *) = open;
> > > ```
> > > If the attribute is a type attribute as opposed to a declaration
> > > attribute, then you can think about diagnostics in these sort of cases.
> > > However, if it is a type attribute, there is more work involved in
> > > hooking it up to the type system.
> > >
> > > FWIW, this attribute smells like it should be a type attribute rather
> > > than a declaration attribute because I can imagine wanting to use this
> > > with function pointers and not losing the analysis capabilities (esp for
> > > things like callback functions).
> > I see your point. I have, however, some concerns making this part of the
> > type system. I think it would be great to let the users add the annotations
> > gradually without any diagnostic. For example:
> >
> > ```
> > void my_function_with_callback( int (*callback)(int
> > __attribute__((handle_use)) );
> >
> > ...
> >
> > my_function_with_callback(already_annotated);
> > my_function_with_callback(not_yet_annotated); // Do we intend to give a
> > diagnostics here?
> > ```
> >
> > What do you think?
> After some digging it looks like I cannot apply any attributes to lambdas.
> For example, you cannot even add `[[noreturn]]` to a lambda, because the
> language only supports type attributes in that position.
> After some digging it looks like I cannot apply any attributes to lambdas.
That's what got me thinking about making this a type attribute instead. It
would be a shame for you to not be able to mark lambdas.
> I have, however, some concerns making this part of the type system.
Understandable -- type attributes are not much fun. ;-) I think your concern is
justified, but you have the control to allow or disallow whatever you'd like in
terms of the semantics of the type attribute. Personally, I would want that use
case to give a diagnostic because that use may close the handle, for instance.
Or it could be creating/releasing a handle instead of just using one, etc.
For instance, one idea (and it may not be a good one) is that you could
diagnose any implicit conversion between the function pointer types, but still
allow explicit conversion between them. This gives users a transition path --
they can explicitly add the type cast in the places they've not yet got the
annotations added yet. They could even use a macro to perform the cast, giving
them a handy marker of what code still needs to be fixed.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6524
+ if (auto *PVD = dyn_cast<ParmVarDecl>(D)) {
+ if (PVD->getType()->isIntegerType()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)
----------------
xazax.hun wrote:
> aaron.ballman wrote:
> > I'm skeptical of this. I think a better check is if the type is a pointer
> > or reference -- was there a reason you didn't go with that approach?
> The reason why I do not want to restrict this to pointers and references is
> that the users might have a custom wrapper type to deal with handles that
> might be passed by value. Or the user might pass an iterator by value which
> points to a handle. These are not extremely likely, but I did not want to
> diagnose those cases. What do you think?
I guess I was envisioning the value type being the issue, but I suppose you
could have some sort of reference-counted object where the copy is not
problematic. But this is still a bit of a strange constraint -- it disallows
putting it on an integer parameter type, but not, say a float or a `const char
*`?
Have you considered requiring the handle type to be annotated itself, so that
you can verify the parameter is actually a handle as opposed to a mistake?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70469/new/
https://reviews.llvm.org/D70469
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
