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

[Martin Böhme via Phabricator via cfe-commits]

mboehme marked 4 inline comments as done.
mboehme added inline comments.


================
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
----------------
xazax.hun wrote:
> 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. 
I'm hestitant do to this, as the doc is likely to become out of date at some point. We've already discussed plans on another patch to eliminate `StructValue` entirely, for example. So I'd rather make sure we have sufficient documentation with the code itself, where it's most likely to be kept up to date.

Anything in particular from the doc that you think would be worth capturing here? (I intend to submit shortly, but would address this in a followup patch.)


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:678
+  if (StorageLocation *ExistingLoc =
+          getStorageLocation(RecordPRValue, SkipPast::None))
+    return *cast<AggregateStorageLocation>(ExistingLoc);
----------------
xazax.hun wrote:
> Is eliminating `SkipPast` coming in a follow-up patch?
Yes! I've already got a draft patch, which I'll be submitting for review soon.


================
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;
----------------
xazax.hun wrote:
> 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?
Hm, interesting idea! This definitely sounds like a way to avoid mistakes, though there would definitely be some details to work out -- for example, a `StorageLocation` for the arguments only works if they are passed by reference, not by value. Something to think about for sure though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155446