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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
