[PATCH] D124807: [clang][dataflow] Only skip ExprWithCleanups when visiting terminators

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:230
+  ///
+  /// Requirements:
+  ///
----------------
sgatev wrote:
> Why add these as requirements instead of skipping past `ExprWithCleanups` internally, as we currently do for parens? Should we add similar requirements to `createStorageLocation`, `setStorageLocation`, and `getStorageLocation`? Would that result in lots of client code sprinkled with `IgnoreParens` and some form of `ignoreExprWithCleanups`?
Ah, I missed that we do this automatically for parens, in part because we //do// unnecessarily sprinkle `IgnoreParens` in a lot of places in `Transfer.cpp`.

That said, we can't avoid sprinkling them at least in some places for the time being. Fixing this greatly expands beyond the scope of fixing a `cast` assert firing. (That is still, aspirationally, the goal of this patch.)

Let me remove the `ParenExpr` requirement (that's incorrect), leave the `ExprWithCleanups` requirement (with `FIXME`), and leave the holistic cleanup for a separate patch.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:429-430
 Value *Environment::getValue(const Expr &E, SkipPast SP) const {
+  assert(!isa<ParenExpr>(&E));
+  assert(!isa<ExprWithCleanups>(&E));
   auto *Loc = getStorageLocation(E, SP);
----------------
sgatev wrote:
> 
Acknowledged. Obviated by removing the `ParenExpr` assert.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:224
+  if (auto *OptionalVal = cast_or_null<StructValue>(State.Env.getValue(
+          *ObjectExpr->IgnoreParens(), SkipPast::ReferenceThenPointer))) {
     auto *HasValueVal = getHasValue(OptionalVal);
----------------
sgatev wrote:
> Do we have a test that fails if `IgnoreParens` is missing? If not, let's add a test or extend one of the existing. Perhaps https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp#L1286
I missed that we ignore parens automatically in `getValue`. Removing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124807