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

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Transforms/Utils/NameAnonFunctions.cpp:59
@@ +58,3 @@
+  auto ModuleHash = moduleHash(M);
+  Twine Prefix = Twine("anon.") + ModuleHash + ".";
+  int count = 0;
----------------
joker.eph wrote:
> 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...).
Reading through http://llvm.org/docs/ProgrammersManual.html#dss-twine, I am not convinced the lifetime of the "anon." temporary will be extended.

If it is legal, why wrap the "anon." part in a Twine and not "."?

We're already doing the expensive part (computation of the ModuleHash string) unconditionally, I'm not sure it is worth trying to optimize this part. Especially since the Twine usage makes me nervous, but I could be missing something...

================
Comment at: lib/Transforms/Utils/NameAnonFunctions.cpp:64
@@ +63,3 @@
+      continue;
+    F.setName(Prefix + Twine(count++));
+    Changed = true;
----------------
joker.eph wrote:
> 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.
Actually, this could be: 
Twine("anon.") + ModuleHash + Twine(".") + Twine(count++)


http://reviews.llvm.org/D18883