ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.
This is really impressive. Thank you!
================
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.
+ ///
----------------
mboehme wrote:
> 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?
Looks great!
================
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.
+ ///
----------------
mboehme wrote:
> ymandel wrote:
> > which constructor? Also, is this clause clarifying the "other fields cannot
> > change" part of the sentence?
> Reworded. Is this clearer now?
Yes. thx
================
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);
----------------
mboehme wrote:
> 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)`.
Thanks, that clarifies. I missed the "in different environments" bit. Makes
sense now.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:466
+ return;
+ Env.setStorageLocationStrict(*S, *MemberLoc);
}
----------------
This diff makes me very happy. :)
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:496
+ Env.getValue(*Arg, SkipPast::Reference))) {
+ if (auto *Val = cast<StructValue>(Env.createValue(S->getType()))) {
+ Env.setValueStrict(*S, *Val);
----------------
I thought `cast` requires a nonnull argument, so this will always be true?
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:397
+ // TODO: Instead of these case distinctions, we would ideally want to be able
+ // to simply use `Environment::createObject()` here, the same way that we do
----------------
FIXME, here and below.
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:30
+// TODO: There are still remaining checks here that check for consistency
+// between `StructValue` and `AggregateStorageLocation`. Now that the
redundancy
----------------
fixme
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
