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

[Martin Böhme via Phabricator via cfe-commits]

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