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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/CalledOnceCheck.h:32
+/// \enum SwitchSkipped -- there is no call if none of the cases appies.
+/// \enum LoopEntered -- no call when the loop is entered.
+/// \enum LoopSkipped -- no call when the loop is not entered.
----------------
NoQ wrote:
> Hold up, how can this happen at all without path sensitivity? If the loop is not entered then literally nothing happens, so there can't be a call. If there's no call in either case then probably it's just "there's simply no call at all"?
You can see examples in tests 😉 


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:541-542
+private:
+  NotCalledClarifier(const CFGBlock *Parent, const CFGBlock *SuccInQuestion)
+      : Parent(Parent), SuccInQuestion(SuccInQuestion) {}
+
----------------
NoQ wrote:
> This is one of the more subtle facilities, i really want a comment here a lot more than on the `NamesCollector`. Like, which blocks do you feed into this class? Do i understand correctly that `Parent` is the block at which we decided to emit the warning? And `SuccInQuestion` is its successor on which there was no call? Or on which there was a call? I can probably ultimately figure this out if i read all the code but i would be much happier if there was a comment.
These are correct questions and I will add a comment, but I want to point out that these questions should be asked not about a private constructor (I always consider those as "don't go there girlfriend!").  The only "entry point" to this class,`clarify`, has more reasonable parameter names.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:560
+                    bool CheckConventionalParameters)
+      : FunctionCFG(FunctionCFG), AC(AC), Handler(Handler),
+        CheckConventionalParameters(CheckConventionalParameters),
----------------
NoQ wrote:
> Why not `FunctionCFG(AC->getCFG())` and omit the CFG parameter? That's the same function, right?
Right, I thought about it!  It is simply how it was done in some other analysis-based warning


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:780
+    //
+    // The following algorithm has the worst case complexity of O(N + E),
+    // where N is the number of basic blocks in FunctionCFG,
----------------
NoQ wrote:
> I'm super used to `V` and `E` for Vertices and Edges in the big-O-notation for graph algorithms, is it just me?
Yep, it is a rudiment of the previous version of this comment.  I'll change it!


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:822
+  /// calling the parameter itself, but rather uses it as the argument.
+  template <class CallLikeExpr>
+  void checkIndirectCall(const CallLikeExpr *CallOrMessage) {
----------------
NoQ wrote:
> Did you consider `AnyCall`? That's a universal wrapper for all kinds of AST calls for exactly these cases. It's not super compile-time but it adds a lot of convenience. (Also uh-oh, its doxygen page seems to be broken). It's ok if you find it unsuitable but i kind of still want to popularize it.
It doesn't seem to have iteration over arguments.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1124
+                     (ReturnChildren &&
+                      ReturnChildren->getParent(SuspiciousStmt));
+            }
----------------
NoQ wrote:
> 
Oh, didn't notice that one!


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1132
+  /// Check if parameter with the given index was ever used.
+  /// NOTE: This function checks only for escapes.
+  bool wasEverUsed(unsigned Index) const {
----------------
NoQ wrote:
> So the contract of this function is "We haven't seen any actual calls, which is why we're wondering", otherwise it's not expected to do what it says it does?
Right!


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1247
+                               State &ToAlter) const {
+    // Even when the analyzer is technically correct, it is a widespread pattern
+    // not to call completion handlers in some scenarios.  These usually have
----------------
NoQ wrote:
> (:
So true 😄 


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1310
+  /// a specified status for the given parameter.
+  bool isAnyOfSuccessorsHasStatus(const CFGBlock *Parent,
+                                  unsigned ParameterIndex,
----------------
NoQ wrote:
> "Successor is has status" doesn't sound right, maybe `anySuccessorHasStatus`?
Dern it! I noticed that before, but forgot to change it, thanks!


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1442
+    // is intentionally not called on this path.
+    if (Cast->getType() == AC.getASTContext().VoidTy) {
+      checkEscapee(Cast->getSubExpr());
----------------
NoQ wrote:
> I feel really bad bringing this up in this context but i guess it kind of makes sense to canonicalize the type first?
Will do


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92039