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