[PATCH] D74813: [RFC] Add hash of block contents to function block names

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

The demangler changes should also be made in `libcxxabi/src/demangle`, as there is a copy of the demangler there. This change also needs tests, there should be demangler tests (in libcxxabi/test/test_demangle.cpp), and IRGen tests (in clang/test/CodeGen) that check the mangled name. Finally, can you please add full contexts to your diffs? (by running e.g. `git show -U99999`).



================
Comment at: clang/lib/AST/Mangle.cpp:39
                                 raw_ostream &Out) {
-  unsigned discriminator = Context.getBlockId(BD, true);
-  if (discriminator == 0)
-    Out << "__" << Outer << "_block_invoke";
-  else
-    Out << "__" << Outer << "_block_invoke_" << discriminator+1;
+  Out << "__" << Outer << "_block_invoke";
+
----------------
We should also probably do this for global blocks, which are handled separately below.


================
Comment at: clang/lib/AST/Mangle.cpp:42
+  ArrayRef<ParmVarDecl *> params = BD->parameters();
+  for(unsigned i = 0; i < params.size(); i++) {
+    ParmVarDecl *param = params[i];
----------------
Might be cleaner as a range-for: `for (ParmVarDecl *PVD : BD->parameters)`


================
Comment at: clang/lib/AST/Mangle.cpp:46
+  }
+  llvm::raw_svector_ostream *Out2 = (llvm::raw_svector_ostream*)&Out;
 }
----------------
I'm not sure why Out2 exists.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:5537
+
+      paramType->print(ParamOS);
+    }
----------------
Can you make a special BlockInvocationFunction AST node that stores the parameter types as AST nodes (rather than strings) in a NodeArray? Right now it looks like this code is leaking, since initializeOutputStream allocates a buffer that it expects the user of OutputStream to manage (the ownership is a bit awkward, but its meant to accommodate the API of `__cxa_demangle`). 


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

https://reviews.llvm.org/D74813