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

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

xazax.hun 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
----------------
mboehme wrote:
> 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.)
I have nothing I would miss from the comments, but there were more code examples in the design doc that helped me understand the problem. I can imagine some people have easier time consuming that document for this reason. But I agree that this is not a strong enough argument to link to something that might go stale in the future. 


================
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;
----------------
mboehme wrote:
> 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.
Another potential advantage of that approach would be to amortize the lookup costs in case there are multiple checks participating in the same fixed-point iteration. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155446