[PATCH] D155446: [clang][dataflow] Eliminate duplication between `AggregateStorageLocation` and `StructValue`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.

This is really impressive. Thank you!



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:337-338
 
+  /// Returns the location of the result object that a prvalue `E` of record
+  /// type initializes.
+  ///
----------------
mboehme wrote:
> ymandel wrote:
> > I found this sentence hard to parse. Lots of "of..." chained. I think it might be clearer with a bit of a preamble about record prvalues. Or, refer the reader to Value.h for details? Once I read that, this became a lot clearer.
> Reworded.
> 
> I've tried to avoid referring to Value.h because `StructValue` itself may be going away entirely in the not-too-distant future. Instead, I've adapted some material from there (which also means people don't need to cross-reference Value.h).
> 
> WDYT?
Looks great!


================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:132
+  /// All other fields cannot change their storage location; the final value of
+  /// the storage location must be initialized using the constructor.
+  ///
----------------
mboehme wrote:
> ymandel wrote:
> > which constructor? Also, is this clause clarifying the "other fields cannot change" part of the sentence?
> Reworded. Is this clearer now?
Yes. thx


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:807-809
+    // As we already have a storage location for the `StructValue`, we can and
+    // should associate them in the environment.
+    setValue(Loc, StructVal);
----------------
mboehme wrote:
> ymandel wrote:
> > how does this interact with the fact that multiple StructValues can share the same ASL?
> Not sure exactly what you're asking?
> 
> First of all, I think the wording on the documentation for `StructValue` may have been confusing, so I've reworded that to make it explicit that when I say that multiple `StructValue`s can share the same `AggregateStorageLocation`, what I mean, more precisely, is that the same `AggregateStorageLocation` can be associated with different `StructValue`s in different environments.
> 
> Here, however, we've only just created the `AggregateStorageLocation` (and a `StructValue` to go with it), so it definitely makes sense to associate the two through a call to `setValue(Loc, StructVal)`.
Thanks, that clarifies. I missed the "in different environments" bit. Makes sense now.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:466
+      return;
+    Env.setStorageLocationStrict(*S, *MemberLoc);
   }
----------------
This diff makes me very happy. :)


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:496
+                     Env.getValue(*Arg, SkipPast::Reference))) {
+        if (auto *Val = cast<StructValue>(Env.createValue(S->getType()))) {
+          Env.setValueStrict(*S, *Val);
----------------
I thought `cast` requires a nonnull argument, so this will always be true?


================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:397
 
+  // TODO: Instead of these case distinctions, we would ideally want to be able
+  // to simply use `Environment::createObject()` here, the same way that we do
----------------
FIXME, here and below.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:30
 
+// TODO: There are still remaining checks here that check for consistency
+// between `StructValue` and `AggregateStorageLocation`. Now that the redundancy
----------------
fixme


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155446