[PATCH] D18883: Add a pass to name anonymous/nameless function

[Mehdi AMINI via llvm-commits]

joker.eph added inline comments.

================
Comment at: lib/Transforms/Utils/NameAnonFunctions.cpp:31
@@ +30,3 @@
+      continue;
+    if (!F.hasName())
+      continue;
----------------
george.burgess.iv wrote:
> Nit: Can this just be folded into `if (F.isDeclaration() || F.hasLocalLinkage() || !F.hasName())`?
Thanks, good point, forgot to clean this up...

================
Comment at: lib/Transforms/Utils/NameAnonFunctions.cpp:59
@@ +58,3 @@
+  auto ModuleHash = moduleHash(M);
+  Twine Prefix = Twine("anon.") + ModuleHash + ".";
+  int count = 0;
----------------
tejohnson wrote:
> george.burgess.iv wrote:
> > ISTM this code is making many `Twine`s here, and the lifetimes of all but the top-level `Twine` will end when we're done constructing `Prefix`, which is bad. Am I missing something?
> Right, I think this should just be std::string (although the Twine(count++) below seems fine since it is constructing a function parameter).
George: I don't see what is the issue here: the Twine are *copied*. The lifetime we have to look at is the lifetime of the underline objects every Twine wraps.

Teresa: this is a good suggestion in general: serializing the prefix once in a string here should be better than doing it multiple time in the loop as part of the name. The catch here is that I don't expect the loop to be large in average (is unnamed function a common thing?), and serializing early means unconditionally (not that I think it matters here...).

================
Comment at: lib/Transforms/Utils/NameAnonFunctions.cpp:64
@@ +63,3 @@
+      continue;
+    F.setName(Prefix + Twine(count++));
+    Changed = true;
----------------
tejohnson wrote:
> Another possibility would be to move the Twines here: 
>  Twine("anon.") + ModuleHash + "." + Twine(count++)
I believe this is NFC, but loses the "string optimization" you mentioned above.


http://reviews.llvm.org/D18883