gribozavr2 added inline 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). ---------------- Shouldn't it go the other way, from Src to Dst? ================ 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. +/// ---------------- 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". ================ Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:391 +llvm::Error +runDataflowReturnError(llvm::StringRef Code, VerifyResultsT VerifyResults, + DataflowAnalysisOptions Options, ---------------- 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. 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
