ymandel added a comment.
I got up to Transfer.cpp and figured I should send along what I have. Looks
very good so far! I should have the rest of the review in a few hours...
================
Comment at:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:337-338
+ /// Returns the location of the result object that a prvalue `E` of record
+ /// type initializes.
+ ///
----------------
I found this sentence hard to parse. Lots of "of..." chained. I think it might
be clearer with a bit of a preamble about record prvalues. Or, refer the reader
to Value.h for details? Once I read that, this became a lot clearer.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:340
+ ///
+ /// TODO: Currently, this simply returns a stable storage location for `E`,
+ /// but this doesn't do the right thing in scenarios like the following:
----------------
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:353
+ /// `E` must be a prvalue of record type.
+ AggregateStorageLocation &getResultObjectLocation(const Expr &E);
+
----------------
Given that this is specific to record types, and that's not reflected in the
argument type, should the name somehow reflect that?
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:406
/// Creates a value appropriate for `Type`, if `Type` is supported, otherwise
- /// return null. If `Type` is a pointer or reference type, creates all the
- /// necessary storage locations and values for indirections until it finds a
+ /// return null.
+ ///
----------------
================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:97-99
+ for (auto [Field, Loc] : Children) {
+ assert(Field->getType()->isReferenceType() || Loc != nullptr);
+ }
----------------
When assertions are off this will be an odd for loop with no body. Will the
optimizer delete it? If not, then consider putting the loop in a lambda inside
the assert (or somesuch).
================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:132
+ /// All other fields cannot change their storage location; the final value of
+ /// the storage location must be initialized using the constructor.
+ ///
----------------
which constructor? Also, is this clause clarifying the "other fields cannot
change" part of the sentence?
================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:202-223
+/// In C++, prvalues of class type serve only a limited purpose: They can only
+/// be used to initialize a result object. It is not possible to access member
+/// variables or call member functions on a prvalue of class type.
+/// Correspondingly, `StructValue` also serves only two limited purposes:
+/// - It conveys a prvalue of class type from the place where the object is
+/// constructed to the result object that it initializes.
+/// When creating a prvalue of class type, we already need a storage location
----------------
These comments are fantastic!
================
Comment at: clang/include/clang/Analysis/FlowSensitive/Value.h:206-207
+/// Correspondingly, `StructValue` also serves only two limited purposes:
+/// - It conveys a prvalue of class type from the place where the object is
+/// constructed to the result object that it initializes.
+/// When creating a prvalue of class type, we already need a storage location
----------------
nit: consider adding a simple example. Is this one right?
`MyStruct S = MyStruct(3);`
The intiializer is a prvalue which will result in a StructValue wrapping an
ASL. Then, we'll model the decl `S` using that ASL.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:127
+
+ assert(&StructVal1->getAggregateLoc() == &StructVal2->getAggregateLoc());
+
----------------
Please add comment explaining why we can assume this invariant.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:129
+
+ MergedVal = &MergedEnv.create<StructValue>(StructVal1->getAggregateLoc());
+ } else {
----------------
Why do we need a new StructValue rather than reusing one of the existing ones?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:682-684
if (auto *StructVal = dyn_cast<StructValue>(&Val)) {
- auto &AggregateLoc = *cast<AggregateStorageLocation>(&Loc);
-
- const QualType Type = AggregateLoc.getType();
- assert(Type->isRecordType());
-
- for (const FieldDecl *Field : DACtx->getModeledFields(Type)) {
- assert(Field != nullptr);
- StorageLocation &FieldLoc = AggregateLoc.getChild(*Field);
- MemberLocToStruct[&FieldLoc] = std::make_pair(StructVal, Field);
- if (auto *FieldVal = StructVal->getChild(*Field))
- setValue(FieldLoc, *FieldVal);
- }
+ assert(&StructVal->getAggregateLoc() == &Loc);
}
----------------
Maybe just an assert?
```
assert(!isa<StructValue>(&Val) || &cast<StructValue>(&Val)->getAggregateLoc()
== &Loc);
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:690
void Environment::clearValue(const StorageLocation &Loc) {
LocToVal.erase(&Loc);
}
----------------
is it worth exposing this in the header (inline) now that it's just one line?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:704
+ setStorageLocation(E, StructVal->getAggregateLoc());
+ assert(getValue(StructVal->getAggregateLoc()) == &Val);
+ return;
----------------
How does this work, given that you never called `setValue`?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:807-809
+ // As we already have a storage location for the `StructValue`, we can and
+ // should associate them in the environment.
+ setValue(Loc, StructVal);
----------------
how does this interact with the fact that multiple StructValues can share the
same ASL?
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:817
+StorageLocation &
+Environment::createFieldOrPointeeLoc(QualType Ty,
----------------
Consider adding a comment for this given that there's none in the header. the
name is a bit confusing on its own since the function also creates a
corresponding value, not just the loc.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:353
return nullptr;
- auto &ValueLoc = Env.createStorageLocation(Ty);
- Env.setValue(ValueLoc, *ValueVal);
+ auto &ValueLoc = Env.createObject(Ty);
auto &ValuePtr = Env.create<PointerValue>(ValueLoc);
----------------
What happens to `ValueVal` if we use `createObject`?
================
Comment at: clang/lib/Analysis/FlowSensitive/RecordOps.cpp:91-92
+ return false;
} else {
- if (Env1.getValue(*FieldLoc1) != Env2.getValue(FieldLoc2))
+ if (Env1.getValue(*FieldLoc1) != Env2.getValue(*FieldLoc2))
return false;
----------------
nit: use `else if` ?
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
