mboehme marked 3 inline comments as done. mboehme added a comment. https://reviews.llvm.org/D154339 has changes in response to post-commit comments.
================ Comment at: clang/include/clang/Analysis/FlowSensitive/RecordOps.h:26 +/// fields of record type. It also copies properties from the `StructValue` +/// associated with `Dst` to the `StructValue` associated with `Src` (if these +/// `StructValue`s exist). ---------------- gribozavr2 wrote: > Shouldn't it go the other way, from Src to Dst? Oops. Fixed in https://reviews.llvm.org/D154339. ================ Comment at: clang/include/clang/Analysis/FlowSensitive/RecordOps.h:53 +/// refer to the same storage location. If `StructValue`s are associated with +/// `Loc1` and `Loc2`, it also compares the properties on those `StructValue`s. +/// ---------------- gribozavr2 wrote: > Could you add a caveat that only 'true' return values from this function > matter? > > That is, "false" means "might or might not be equal at runtime, we don't > know". Good point. Done in https://reviews.llvm.org/D154339. ================ Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:391 +llvm::Error +runDataflowReturnError(llvm::StringRef Code, VerifyResultsT VerifyResults, + DataflowAnalysisOptions Options, ---------------- gribozavr2 wrote: > Could you change VerifyResultsT to a std::function (like in other overloads > above) so that the type signature is clear? > > I'm also unsure about the name - why start a new overload set? It seems like > another variant of the checkDataflow() functions above. > Could you change VerifyResultsT to a std::function (like in other overloads > above) so that the type signature is clear? Done in https://reviews.llvm.org/D154339. I could have sworn that when I moved this from TransferTest.cpp to here that I tried making this change and realized I couldn't for some reason. But obviously I was wrong. > I'm also unsure about the name - why start a new overload set? It seems like > another variant of the checkDataflow() functions above. This is really just moved from TransferTest.cpp -- and it's more closely related to the `runDataflow()` functions there and in the newly added RecordOps.cpp. (I can't call it `runDataflow()` because then it would differ only in return type from one of the functions in the overload set.) Let's continue the discussion on https://reviews.llvm.org/D154339 if necessary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153006/new/ https://reviews.llvm.org/D153006 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
