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

[George Burgess IV via llvm-commits]

george.burgess.iv 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:
> > 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...
> (Also we may not want to compute the hash unconditionally)
> 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.

I agree with Teresa, because when hooking a `Twine` up to another `Twine`, we call [Twine::concat](http://llvm.org/docs/doxygen/html/Twine_8h_source.html#l00485), which stores the `this` pointer (and the pointer to the `concat` argument) in the newly-created `Twine`. Given that a single `Twine` can only hook two things together, we'll need at least two of them here to hook all three parts of this string together.


http://reviews.llvm.org/D18883