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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
