xazax.hun accepted this revision. xazax.hun added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:78 +/// and integers in the framework. +static const Expr *ignoreParenImpCastsExceptToBool(const Expr *E) { + const Expr *LastE = nullptr; ---------------- li.zhe.hua wrote: > sgatev wrote: > > xazax.hun wrote: > > > li.zhe.hua wrote: > > > > sgatev wrote: > > > > > I don't recall why we need to ignore implicit casts here. Can't we > > > > > ignore parens and rely on the built-in transfer function, possibly > > > > > adding handling of some missing casts there? > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L192 > > > > If we only ignore parens, a test in the optional checker tests begins > > > > to fail, specifically `UncheckedOptionalAccessTest.ValueOrComparison`. > > > > The missing "cast" is an `ExprWithCleanups`. I didn't know how to deal > > > > with that, so this patch just working around the assert. > > > In general, I prefer to handle as much as possible with transfer > > > functions and skip as little as possible in the AST. We might skip > > > `ExprWithCleanups` nodes today, but we will need them tomorrow to > > > properly model where certain destructors are being invoked. > > We already have `skipExprWithCleanups` [1]. I wonder if it makes sense to > > replace that with a simple transfer function like the one for the `CK_NoOp` > > implicit cast. Would that solve the problem and remove the need for > > `ignoreParenImpCastsExceptToBool`? In the future we might replace that > > transfer function with a proper one, as Gábor suggested. > > > > [1] > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L36 > I sat down with @ymandel to get a better understanding of what was happening > here, which was really helpful in understanding why my random flailing was > making tests pass, and what I believe to be a better way forward. > > So, the CFG does not emit `ParenExpr` nodes. As such, the transfer function > never sees them, and so when we do any kind of manual traversal (e.g. to look > at a sub-expression), we use `ignoreParens()` because they effectively don't > exist. The same thing happens with `ExprWithCleanups`. The CFG doesn't appear > to emit them explicitly, and so we should similarly avoid them when manually > traversing the AST. > > Note: Their position in the AST is slightly more constrained, likely as the > immediate children of `*Stmt` nodes. This means we don't need to sprinkle > these skips as widely as `ignoreParens()`. > > In terms of why adding `VisitFullExpr` "fixed" the failing test: > `extendFlowCondition()` [[ > https://github.com/llvm/llvm-project/blob/ed1b32791dbb4c02cd761742a63cdf1e9d644ae6/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L108 > | manually calls ]] `transfer` on the provided `Cond` expression, which is a > `ExprWithCleanups` if the caller uses `ignoreParens()` instead of > `ignoreParenImpCasts()`. > > --- > > @xazax.hun: > > > In general, I prefer to handle as much as possible with transfer functions > > and skip as little as possible in the AST. We might skip `ExprWithCleanups` > > nodes today, but we will need them tomorrow to properly model where certain > > destructors are being invoked. > > The CFG already emits calls to destructors as a result of `ExprWithCleanups` > ([[ https://godbolt.org/z/fo846dcEa | godbolt ]]), so skipping them will not > cause us to miss calls to destructors. > > --- > > @sgatev: > > > We already have `skipExprWithCleanups` [1]. I wonder if it makes sense to > > replace that with a simple transfer function like the one for the `CK_NoOp` > > implicit cast. Would that solve the problem and remove the need for > > `ignoreParenImpCastsExceptToBool`? In the future we might replace that > > transfer function with a proper one, as Gábor suggested. > > > > [1] > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L36 > > I believe that `ParenExpr` and `ExprWithCleanups` are categorically the same > within the framework (roughly, nodes that are not emitted by the CFG) and so > we should handle them in the same way. The strategy right now for `ParenExpr` > is to skip them, so I am inclined to do so as well with `ExprWithCleanups` > for now. (I'm not necessarily opposed to having `ParenExpr` and > `ExprWithCleanups` nodes being handled in the transfer function, but there's > a question of how to get them in there if the CFG isn't going to emit them.) > > That would mean //more// usage of `skipExprWithCleanups`, and is reflected in > this most recent revision. Thanks for the detailed explanation, this sounds good to me! Could you add a short summary as a comment to `ignoreExprWithCleanups`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124807/new/ https://reviews.llvm.org/D124807 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits