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