[PATCH] D153006: [clang][dataflow] Perform deep copies in copy and move operations.

[Dmitri Gribenko via Phabricator via cfe-commits]

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