vsavchenko marked an inline comment as done.
vsavchenko added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1774
+ let LangOpts = [ObjC];
+ let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> No new undocumented attributes, please.
Sure, will do!
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3623
+static void handleCalledOnceAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+ if (D->isInvalidDecl())
+ return;
----------------
aaron.ballman wrote:
> I don't think there's a need for this check -- attaching the attribute to an
> invalid declaration doesn't hurt anything downstream (I believe), but failing
> to attach the attribute harms AST fidelity for the folks doing work on
> retaining erroneous AST nodes.
Sure, simply did it like it's done for some other attribute for parameters.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3630
+ if (!isFunctionLike(*T)) {
+ S.Diag(AL.getLoc(), diag::err_called_once_attribute_wrong_type);
+ return;
----------------
aaron.ballman wrote:
> Because there can be multiple parameters in the declaration, passing the
> range to the specific problematic parameter is helpful.
I was just thinking that because attribute applies to each parameter separately
and we already show this error at the location of the attribute, it is clear
which parameter is implied.
================
Comment at: clang/test/SemaObjC/warn-called-once.m:887
+}
+@end
----------------
aaron.ballman wrote:
> Can you also add a test that shows this works with lambda or block parameters
> that contain the attribute? e.g.,
> ```
> [](void (*fp CALLED_ONCE)()) { ... }(some_fp);
> ```
> Also, you should add some tests that the attribute diagnoses when applied to
> something other than a parameter, is given an argument, and a test that the
> attribute is rejected unless in ObjC.
There is a test here called `block_with_called_once` that covers that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92039/new/
https://reviews.llvm.org/D92039
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
