[PATCH] D117218: [clang][dataflow] Add transfer functions for constructors

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev marked an inline comment as done.
sgatev added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:114
+    } else if (S->getCastKind() == CK_NoOp) {
+      auto *SubExprLoc = Env.getStorageLocation(*SubExpr, SkipPast::None);
+      if (SubExprLoc == nullptr)
----------------
xazax.hun wrote:
> An alternative way to handle Noop operations would be to make location lookup function always skip certain nodes, so we do not need to store locations for those subexpressions. I don't have a strong feeling for either solution, this is fine as it is, just wanted to be sure that both were considered. 
It makes sense to consider it. Right now I don't have signals that one is better than the other. I think the current implementation fits the general model a bit better. Nevertheless, I added a FIXME to keep this option in mind.


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:277
+  void VisitCallExpr(const CallExpr *S) {
+    if (S->isCallToStdMove()) {
+      assert(S->getNumArgs() == 1);
----------------
xazax.hun wrote:
> Interesting, I only see `isCallToStdMove` in the CallExpr API, although I imagine, `std::forward` has a similar level of importance. 
I haven't looked into `std::forward` yet, but happy to do that in the next patch.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:39
 protected:
+  enum class CppVersion {
+    k14 = 14,
----------------
xazax.hun wrote:
> Shouldn't we piggyback on `clang::LangStandard::Kind`? If it is not easy to convert that to the appropriate command line flag feel free to ignore this.
Makes sense. Thanks for pointing this out!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117218/new/

https://reviews.llvm.org/D117218