NoQ added a comment.
In D82598#2164363 <https://reviews.llvm.org/D82598#2164363>, @Szelethus wrote:
> > Hmm, interesting. I don't really understand why do we need to keep that
> > block live, as we definitely won't use any of the value it provides (since
> > it does not provide a value at all).
>
> Actually, what I said initially is true:
>
> > [...] the only non-expression statements it **queried** are
> > ObjCForCollectionStmts [...]
>
> so I think it'd be okay to simply drop this.
Yeah, that sounds about right; you observed the current behavior to be a
counterexample but found no evidence that the current behavior makes any sense.
P.S. We've found an example where `ObjCForCollectionStmt`s break (i.e., your
recently added assertion gets hit), i'll reduce and think of a patch. Still
shocked this wasn't covered with tests.
================
Comment at: clang/lib/Analysis/LiveVariables.cpp:317-320
for (Stmt *Child : S->children()) {
- if (Child)
- AddLiveStmt(val.liveStmts, LV.SSetFact, Child);
+ if (const auto *E = dyn_cast_or_null<Expr>(Child))
+ AddLiveExpr(val.liveExprs, LV.SSetFact, E);
}
----------------
Szelethus wrote:
> xazax.hun wrote:
> > Szelethus wrote:
> > > ..this part of the code caused the issue. Looking at the related CFG,
> > > ```lang=c++
> > > void test_lambda_refcapture()
> > > [B2 (ENTRY)]
> > > Succs (1): B1
> > >
> > > [B1]
> > > 1: 6
> > > 2: int a = 6;
> > > 3: operator()
> > > 4: [B1.3] (ImplicitCastExpr, FunctionToPointerDecay, void (*)(int &)
> > > const)
> > > 5: [&](int &a) {
> > > a = 42;
> > > }
> > > 6: [B1.5]
> > > 7: [B1.6] (ImplicitCastExpr, NoOp, const class (lambda at
> > > /home/szelethus/Documents/llvm-project/clang/test/Analysis/live-stmts.cpp:183:3))
> > > 8: a
> > > 9: [B1.7]([B1.8]) (OperatorCall)
> > > 10: clang_analyzer_eval
> > > 11: [B1.10] (ImplicitCastExpr, FunctionToPointerDecay, void (*)(_Bool))
> > > 12: a
> > > 13: [B1.12] (ImplicitCastExpr, LValueToRValue, int)
> > > 14: 42
> > > 15: [B1.13] == [B1.14]
> > > 16: [B1.11]([B1.15])
> > > Preds (1): B2
> > > Succs (1): B0
> > >
> > > [B0 (EXIT)]
> > > Preds (1): B1
> > > ```
> > > its clear that element 5 added the live statement, and I think that that
> > > this entire CFG just simply isn't right. Shouldn't we have a distinct
> > > element for the assignment?
> > >
> > > Shouldn't we have a distinct element for the assignment?
> >
> > Strictly speaking, we have CFGs for a function. The assignment is **not**
> > in this function, it is in the `operator()` of the class representing this
> > lambda expression.
> >
> > So basically, we do have a `LambdaExpr` to represent the expression, but
> > the body of the lambda is in a separate entity.
> Well, `debug.DumpCFG` definitely doesn't indulge me with a separate lambda
> CFG, so I figured this is a (rightful) optimization or compression.
>
> My point is, this entire code snippet is a seriously error prone, best-effort
> heuristic. Switch casing every small little corner case might be tedious,
> troublesome in terms of scaling, and I for sure don't want to maintain it
> 'til eternity, but we have to acknowledge that this isn't a perfect solution
> either.
> Well, debug.DumpCFG definitely doesn't indulge me with a separate lambda CFG
Does that mean that `checkASTCodeBody` doesn't get run on lambda bodies, and
neither do any of our path-insensitive checks?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82598/new/
https://reviews.llvm.org/D82598
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
