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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 15 13:26:40 PST 2020


aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5095
+Clang implements a check for ``called_once`` parameters,
+``-Wcalled-once-parameter``.  It is on by default and finds the following
+violations:
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:5098-5102
+* Parameter is not called at all.
+
+* Parameter is called more than once.
+
+* Parameter is not called on one of the execution paths.
----------------
I think you may have to remove the newlines here to make this a single bulleted list in Sphinx.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:5110
+  void doWithCallback(void (^callback)(void) __attribute__((called_once))) {
+      // callback should be called exactly once before the function returns
+  }
----------------
I think it'd be helpful to show an example where the attribute is used correctly and another one where the parameter is diagnosed.

I think it'd also be helpful to explain why someone should write this attribute on their own declaration. As it stands, it sort of sounds like the attribute is only useful to the author of a method to help double-check that they've written their method properly.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3630
+  if (!isFunctionLike(*T)) {
+    S.Diag(AL.getLoc(), diag::err_called_once_attribute_wrong_type);
+    return;
----------------
vsavchenko wrote:
> 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.
That's a good point; I think it's fine as-is. I think I was thinking about this as a function attribute by accident. :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92039/new/

https://reviews.llvm.org/D92039



More information about the cfe-commits mailing list