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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

A few random comments here and there as i slowly wrap my head around the overall analysis algorithm.



================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:47
+using CFGSizedVector = llvm::SmallVector<T, EXPECTED_NUMBER_OF_BASIC_BLOCKS>;
+constexpr llvm::StringLiteral CONVENTIONAL_NAME = "completionHandler";
+constexpr llvm::StringLiteral CONVENTIONAL_CONDITIONS[] = {
----------------
Around this point I would have given up and declared `using namespace llvm;`.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:76
+
+    // Defined elements should maintain the following properties:
+    //   1. for any Kind K: NoReturn | K == K
----------------
Maybe `static_assert` at least some of those?


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:135
+
+class State {
+public:
----------------
Let's say this out loud: The state is defined as a vector of parameter status values.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:252
+    // procedure for a nested expression instead.
+    const Expr *DeclutteredExpr = E->IgnoreParenCasts();
+    return E != DeclutteredExpr ? Visit(DeclutteredExpr) : nullptr;
----------------
Every time I see such code, I want the behavior with respect to implicit lvalue-to-rvalue casts considered or at least explained explicitly. Because they're the ones that make the expression suddenly represent a completely unrelated value and in this regard they're very different from all other kinds of casts. Like, they're not some immaterial bureaucratic clutter to satisfy the type system, they're a huge transformation, basically one of the most important operations in the entire language, an entire freakin' memory access! So i'm curious how exactly do you want your code to behave in this specific case and i want you to comment loudly about that.

This is also at least the 3rd implementation of this functionality i've seen in Clang (and there are probably a lot more that i haven't seen) but it sounds like nobody's willing to define what they actually want when they do this precisely enough to figure out if they're all doing the same thing or a different thing. Everybody's like "Oh I just want some `DeclRefExpr` to point to, I don't care what role does it play in the expression" but it occasionally turns out that they do in fact care a lot. So, like, i don't insist on doing this right now but it'd be great to finally figure out what causes people to write such code and whether they have something in common.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:421
+  llvm::Optional<Clarification> VisitBinaryOperator(const BinaryOperator *) {
+    // We don't want to report on short-curcuit logical operations.
+    return llvm::None;
----------------
Why not? What if the programmer was a big fan of shell scripts?


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:426
+  llvm::Optional<Clarification> VisitStmt(const Stmt *Terminator) {
+    // If we got here, we didn't have a visit function for more dirived
+    // classes of statement that this terminator actually belongs to.
----------------
Also there's no test for this, right? Otherwise you would have come up with a better message and this case would have been unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92039