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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

I did not do a thorough review checking every line, but I read the design paper and skimmed through this patch. Love the direction, and I am OK with landing this as is.



================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:677
-  // them and the decls of the struct members.
-  llvm::DenseMap<const StorageLocation *,
-                 std::pair<StructValue *, const ValueDecl *>>
----------------
I am glad to see this gone.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:201
 
-/// Models a value of `struct` or `class` type, with a flat map of fields to
-/// child storage locations, containing all accessible members of base struct
-/// and class types.
+/// Models a value of `struct` or `class` type.
+/// In C++, prvalues of class type serve only a limited purpose: They can only
----------------
Should we link to the design doc somewhere? The comments are great and self-contained, but I wonder if there is still some value for the doc to be mentioned somewhere in the code. I do not have a preference, just wondering. 


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:678
+  if (StorageLocation *ExistingLoc =
+          getStorageLocation(RecordPRValue, SkipPast::None))
+    return *cast<AggregateStorageLocation>(ExistingLoc);
----------------
Is eliminating `SkipPast` coming in a follow-up patch?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:534-549
 void transferCallReturningOptional(const CallExpr *E,
                                    const MatchFinder::MatchResult &Result,
                                    LatticeTransferState &State) {
   if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
     return;
 
+  AggregateStorageLocation *Loc = nullptr;
----------------
This is more of a note for the future of these APIs. I wonder if this is the right level of abstraction in this framework. I think it would be great to have something like:
```
void transferCallReturningOptional(const CallExpr *E,
                                   StorageLocation* RetLoc,
                                   ArrayRef<StorageLocation> Args,
                                   const MatchFinder::MatchResult &Result,
                                   LatticeTransferState &State);
```

and all the values and storage locations could come from the framework. It would be great if the user would almost never need to get a value or a location from an expression, they would be readily available. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155446