mboehme marked 16 inline comments as done.
mboehme added inline comments.
================
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.
+ ///
----------------
ymandel wrote:
> 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.
Reworded.
I've tried to avoid referring to Value.h because `StructValue` itself may be
going away entirely in the not-too-distant future. Instead, I've adapted some
material from there (which also means people don't need to cross-reference
Value.h).
WDYT?
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:353
+ /// `E` must be a prvalue of record type.
+ AggregateStorageLocation &getResultObjectLocation(const Expr &E);
+
----------------
ymandel wrote:
> Given that this is specific to record types, and that's not reflected in the
> argument type, should the name somehow reflect that?
Done.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:97-99
+ for (auto [Field, Loc] : Children) {
+ assert(Field->getType()->isReferenceType() || Loc != nullptr);
+ }
----------------
ymandel wrote:
> 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).
Hm -- I'm not sure. Changed to a lambda.
================
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.
+ ///
----------------
ymandel wrote:
> which constructor? Also, is this clause clarifying the "other fields cannot
> change" part of the sentence?
Reworded. Is this clearer now?
================
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
----------------
ymandel wrote:
> 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.
Yes, this looks right. Added this as an example below.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:129
+
+ MergedVal = &MergedEnv.create<StructValue>(StructVal1->getAggregateLoc());
+ } else {
----------------
ymandel wrote:
> Why do we need a new StructValue rather than reusing one of the existing ones?
Added a comment.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:690
void Environment::clearValue(const StorageLocation &Loc) {
LocToVal.erase(&Loc);
}
----------------
ymandel wrote:
> is it worth exposing this in the header (inline) now that it's just one line?
Good point, done.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:704
+ setStorageLocation(E, StructVal->getAggregateLoc());
+ assert(getValue(StructVal->getAggregateLoc()) == &Val);
+ return;
----------------
ymandel wrote:
> How does this work, given that you never called `setValue`?
Agree this is confusing.
The intent here was to check a _pre_condition of the function (i.e. that the
caller must already have associated `StructVal->getAggregateLoc()` with `Val`
in the environment before calling this function) rather than a _post_condition
of the function. But looking at this again, this doesn't seem like a good
contract. All current callers of the function satisfy this precondition, but
it's not documented anywhere, and it's also just simply a strange and
unnecessary precondition. So instead, I've simply replaced this with a
`setValue()` that makes sure this is the case if it wasn't before.
================
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);
----------------
ymandel wrote:
> how does this interact with the fact that multiple StructValues can share the
> same ASL?
Not sure exactly what you're asking?
First of all, I think the wording on the documentation for `StructValue` may
have been confusing, so I've reworded that to make it explicit that when I say
that multiple `StructValue`s can share the same `AggregateStorageLocation`,
what I mean, more precisely, is that the same `AggregateStorageLocation` can be
associated with different `StructValue`s in different environments.
Here, however, we've only just created the `AggregateStorageLocation` (and a
`StructValue` to go with it), so it definitely makes sense to associate the two
through a call to `setValue(Loc, StructVal)`.
================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:817
+StorageLocation &
+Environment::createFieldOrPointeeLoc(QualType Ty,
----------------
ymandel wrote:
> 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.
Yes, the name was bad. Renamed and added a comment in the header.
================
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);
----------------
ymandel wrote:
> What happens to `ValueVal` if we use `createObject`?
Oops -- this was left over from refactoring and was superfluous now. Thanks for
catching. Removed.
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
