[PATCH] D52933: Fix incorrect Twine usage in CFGPrinter

[Jonas Toth via Phabricator via llvm-commits]

JonasToth added inline comments.


================
Comment at: include/llvm/Analysis/CFGPrinter.h:176
+    // Evaluate in a single statement to avoid dangling pointers in a Twine.
+    return ("label=\"W:" + Twine(Weight->getZExtValue()) + "\"").str();
   }
----------------
mcopik wrote:
> JonasToth wrote:
> > lebedev.ri wrote:
> > > JonasToth wrote:
> > > > lebedev.ri wrote:
> > > > > Reading the http://llvm.org/docs/ProgrammersManual.html#dss-twine, i *do* think the fix is correct.
> > > > > The old code would call `str()` on a `Twine` stored on a stack,
> > > > > which is clearly stated as broken in the manual.
> > > > The pattern for Twine would be `Twine("label=\"W:") + Weight->getZExtValue() + "\"").str()`, why use `Twine` for the second value instead of the first one?
> > > > The fix itself should still be correct.
> > > That doesn't compile https://godbolt.org/z/7UhuUJ
> > Fair point :D
> @JonasToth I didn't want to change the original code and only remove the dangling pointer to a stack temporary object. I see that @lebedev.ri already answered your question; I think the problem is that addition operators for Twine are defined only for Twine and StringRef objects. I think there's a conversion to StringRef via ctor for c-string but not for uint64_t.
Yeah. The constructor for Twine is overloaded for all common types, I kinda expected that for the concatentation functionalities, too. Everything is fine as is ;)


https://reviews.llvm.org/D52933