[PATCH] D66404: [CFG] Make destructor calls more accurate

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I have a few nitpicks but i still love this patch, thank you! It picks up the work exactly where i dropped it a year or so ago.

> Respect C++17 copy elision; previously it would generate destructor calls for elided temporaries, including in initialization and return statements.

I think the root cause of these redundant destructors was the confusing AST that contains a `CXXBindTemporaryExpr` even when there's not much of a temporary. See also a very loosely related discussion in http://lists.llvm.org/pipermail/cfe-dev/2018-March/057475.html



================
Comment at: lib/Analysis/CFG.cpp:2102
     case Stmt::CompoundStmtClass:
-      return VisitCompoundStmt(cast<CompoundStmt>(S));
+      return VisitCompoundStmt(cast<CompoundStmt>(S), /*ExternallyDestructed=*/false);
 
----------------
This goes over the 80 characters limit :)


================
Comment at: lib/Analysis/CFG.cpp:2290-2295
+    if (BuildOpts.AddTemporaryDtors) {
+      TempDtorContext Context;
+      VisitForTemporaryDtors(EC->getSubExpr(),
+                             /*ExternallyDestructed=*/true, Context);
+    }
+    return Visit(EC->getSubExpr(), asc);
----------------
This looks like a copy-paste from `VisitExprWithCleanups` with the `ExternallyDestructed` flag flipped. Could you double-check if the update to `asc` that's present in `VisitExprWithCleanups` is relevant here as well? I've actually no idea what do these do and we don't seem to have any tests for them, so feel free to ignore, but it might make sense to at least deduplicate the code.


================
Comment at: test/Analysis/cfg-rich-constructors.cpp:415
 
-// FIXME: There should be no temporary destructor in C++17.
 // CHECK:  return_stmt_with_dtor::D returnTemporary()
----------------
Yay!! I'm very happy these are sorted out.


================
Comment at: test/Analysis/cfg-rich-constructors.mm:63
-// CXX17-NEXT:     6: ~D() (Temporary object destructor)
-// CXX17-NEXT:     7: [B1.5].~D() (Implicit destructor)
 void returnObjectFromMessage(E *e) {
----------------
Whoops, looks like i forgot to add a `FIXME` here. This update is correct, thanks!


================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:3
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++14 -DCXX2A=0 -fblocks -Wall -Wno-unused -Werror %s > %t.14 2>&1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -std=c++2a -DCXX2A=1 -fblocks -Wall -Wno-unused -Werror %s > %t.17 2>&1
+// RUN: FileCheck --input-file=%t.14 -check-prefixes=CHECK,CXX14 -implicit-check-not=destructor %s
----------------
Did you mean `t.20`? :)


================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:50
+// CXX14: (CXXConstructExpr{{.*}}, struct Foo)
+// CXX14: ~Foo() (Temporary object destructor)
+// CHECK: ~Foo() (Implicit destructor)
----------------
Maybe also add `CXX20-NOT` so that to make sure that the destructor *isn't* there? (i think this applies to a lot of other tests)


================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:93
+
+void elided_lambda_capture_init() {
+  // The copy from get_foo() into the lambda should be elided.  Should call
----------------
Pls mention somehow that this language feature is called "generalized lambda captures" because it's fairly hard to google for :)


================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:233
+
+// FIXME: Here are some cases that are currently handled wrongly:
+
----------------
I'm afraid that nobody will notice this comment when more items will be added to this test file. Having a `FIXME` in tests themselves is sufficient.


================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:251-252
+void default_ctor_with_default_arg() {
+  // FIXME: The CFG records a construction of the array type but a destruction
+  // of the base type, which is inconsistent.  It really should should be
+  // emitting a loop, which should also contain the construction/destruction of
----------------
Is it just mismatching dump styles or is it an actual error?


================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:252-254
+  // of the base type, which is inconsistent.  It really should should be
+  // emitting a loop, which should also contain the construction/destruction of
+  // default arguments.
----------------
These should most likely be two separate loops: default arguments are destroyed immediately after the constructor of `qux[2]`, but elements of `qux` should not be destroyed before the end of the scope.


================
Comment at: test/Analysis/more-dtors-cfg-output.cpp:273
+#if CXX2A
+// Boilerplate needed to test co_return:
+
----------------
Feel free to move this into `test/Analysis/inputs/system-header-simulator-cxx.h`, we could always use more mocks in there.


================
Comment at: test/Analysis/temporaries.cpp:836
-#else
-    // FIXME: Destructor called twice in C++17?
-    clang_analyzer_eval(y == 2); // expected-warning{{TRUE}}
----------------
Yay!! I'm very happy these are sorted out.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66404