[PATCH] D157385: [clang][CFG] Cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

For me this looks good, but I'd like @NoQ to sign off on it.



================
Comment at: clang/lib/Analysis/CFG.cpp:1874
+    if (needsAutomaticDestruction(D))
       DeclsNonTrivial.push_back(D);
 
----------------
I'm wondering if you should rename this accordingly.


================
Comment at: clang/lib/Analysis/CFG.cpp:1887-1889
+      bool IsCXXRecordType = (Ty->getAsCXXRecordDecl() != nullptr);
+      if (IsCXXRecordType &&
+          Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
----------------
Here I'd suggest to deduplicate `Ty->getAsCXXRecordDecl()`. Implicit conversion of pointers to bool is idiomatic in LLVM.


================
Comment at: clang/lib/Analysis/CFG.cpp:5307-5308
+    case CFGElement::CleanupFunction:
+    llvm_unreachable("getDestructorDecl should only be used with "
+                     "ImplicitDtors");
     case CFGElement::AutomaticObjectDtor: {
----------------
The unindent doesn't look right to me.


================
Comment at: clang/lib/Analysis/CFG.cpp:5850
 
+  case CFGElement::Kind::CleanupFunction: {
+    OS << "CleanupFunction ("
----------------
Braces shouldn't be needed if you don't declare any variables.


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2429-2438
+
+        case CFGElement::CleanupFunction: {
+          const CFGCleanupFunction &CF = BI.castAs<CFGCleanupFunction>();
+
+          LocksetBuilder.handleCall(nullptr, CF.getFunctionDecl(),
+                                    SxBuilder.createVariable(CF.getVarDecl()),
+                                    CF.getVarDecl()->getLocation());
----------------
Should this be part of a follow-up? (For which you might revive D152504.)


================
Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1428-1429
+// CHECK-NEXT:   4: CFGScopeEnd(i)
+void cleanup_int(int *i) {
+}
+
----------------
For our purposes, a pure declaration might be enough.


================
Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1433
+  int i __attribute__((cleanup(cleanup_int)));
+}
----------------
Ideas for more tests (apart from imitating destructor tests):

* A variable in a block, so that more statements run before the function returns.
* A function with multiple return paths, each of which has to run the cleanup.




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

https://reviews.llvm.org/D157385