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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Somehow only one of my previous inline comment went through?


================
Comment at: lib/Transforms/Utils/NameAnonFunctions.cpp:59
@@ +58,3 @@
+  auto ModuleHash = moduleHash(M);
+  Twine Prefix = Twine("anon.") + ModuleHash + ".";
+  int count = 0;
----------------
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...
You're asking why "anon." is wrapped in a Twine and not ".", this is because otherwise RHS would not operate on Twine at all. And you don't need to wrap "." in a Twine, because it is already implicitly done by the compiler! (here is the clang-ast if you're interested: http://snag.gy/HtMMD.jpg )

That said, with another look at it, George is right: the problem is not the lifetime of the "anon." temporary, the problem is the lifetime of the intermediate `Twine` objects.

The expression I wrote is conceptually the same as:

```
Twine Prefix;
{
  auto TwineAnon = Twine("anon.");
  auto TwineModuleHash = Twine(ModuleHash);
  auto TwineAnonAndModuleHash = TwineAnon + TwineModuleHash;
  auto TwineDot = Twine(".");
  Prefix = TwineAnonAndModuleHash + TwineDot;`
}
// at scope exit the intermediate Twine are dead but Prefix keeps pointers to them
// the underlying objects "anon.", ModuleHash, and "." are still valid.
```



http://reviews.llvm.org/D18883