[PATCH] D92039: [-Wcalled-once-parameter] Introduce 'called_once' attribute

[Valeriy Savchenko via Phabricator via cfe-commits]

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
+}
+ at 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