aaron.ballman added a comment. In D70469#1766084 <https://reviews.llvm.org/D70469#1766084>, @xazax.hun wrote:
> @aaron.ballman > > Type attributes are definitely more flexible as in they can be applied in > more contexts. These attributes, however, make no sense outside of a function > declaration, or maybe a type alias. Function pointers are a primary case for what I was thinking of, and those are neither a function declaration nor a type alias. > So I feel like we could argue on both sides. I made a minimal version of the > attribute so it can potentially unblock dependent revisions (the static > analyzer changes) and plan to add more diagnostics in follow-up patches. What > do you think? Thank you! I think this is a more flexible way forward. In D70469#1770041 <https://reviews.llvm.org/D70469#1770041>, @xazax.hun wrote: > In the meantime, I realized lambdas are actually not that important. Lambdas > are only usable in C++, and in C++ one should prefer to use move only RAII > wrapper for handles. Generally yes, but thinking of things like file descriptors (which are also handles), it seems like overkill to expect someone to wrap those in a separate type. > This reduces the problem into a use after move and we already have a check > for that. These annotations are more important for C, where we do not have > language features to mitigate these problems. The function pointer argument > is still valid though. Okay, that's a fair point. ================ Comment at: clang/include/clang/Basic/Attr.td:3464 + +def AcquireHandle : TypeAttr { + let Spellings = [Clang<"acquire_handle">]; ---------------- These probably should be `DeclOrTypeAttr` because they can be applied to a parameter (which is a declaration) or a type. ================ Comment at: clang/include/clang/Basic/AttrDocs.td:4569 + +def HandleDocs : DocumentationCategory<"Handle Attributes"> { + let Content = [{ ---------------- The docs should clarify when they say "function" that they mean the function type rather than the function declaration. ================ Comment at: clang/lib/Sema/SemaType.cpp:7412 + diag::warn_type_attribute_wrong_type_str) + << Attr.getAttrName()->getName() << "non-function" << CurType; + return; ---------------- You should be able to pass in `Attr` directly instead of `Attr.getAttrName()->getName()`, I believe. Also, I'd prefer the `non-function` be put into the diagnostic text itself with a `%select` if we need to vary it. ================ Comment at: clang/test/Sema/attr-handles.cpp:7 + int __attribute__((acquire_handle)) { return 0; }; +void g(int __attribute__((acquire_handle)) a); // TODO: diagnose this. The acquire attribute only makes sense for outputs. +void h(int __attribute__((acquire_handle(1))) *a); // expected-error {{'acquire_handle' attribute takes no arguments}} ---------------- Are you planning to implement the TODOs as part of this patch? 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
