[PATCH] D130600: [clang][dataflow] Handle return statements

Sam Estep via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 4 05:20:33 PDT 2022

samestep added inline comments.

Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:345
+    // FIXME: Support reference-type returns.
+    assert(Val->getKind() != Value::Kind::Reference);
sgatev wrote:
> Let's do `if (Val->getKind() == Value::Kind::Reference) return;`. Otherwise the framework will be unusable in practice.
Makes sense, will do.

Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:348
+    auto *Loc = Env.getReturnStorageLocation();
+    assert(Loc != nullptr);
+    // FIXME: Model NRVO.
sgatev wrote:
> Let's do `if (Loc == nullptr) return;`
I don't think we want to do that, right? Shouldn't the `return` storage location always be set? Or is this about the "analyzing fragments rather than full functions" thing we discussed yesterday?

Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:564
+      assert(ReturnLoc != nullptr);
+      Env.setStorageLocation(*S, *ReturnLoc);
+      Env.popCall(ExitEnv);
sgatev wrote:
> We use stable storage locations to ensure convergence. In that spirit, shouldn't we assign `ReturnLoc`'s value to `S`'s storage location instead of changing the storage location? Alternatively, we can pass `S`'s storage location to `pushCall` so that it can store it as `ReturnLoc`.
Could you clarify how this hurts convergence? My understanding is that `ReturnLoc` here is already stable, so this would make `S`'s storage location stable too.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list