<div dir="ltr">I can change the return type to std::string tomorrow, if nobody does that first.</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 11, 2017 at 5:26 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Nah, there's only one caller by the looks of it, and it looks like it doesn't need updating if this function changes from StringRef->std::string.</div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 11, 2017 at 8:25 AM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Also agree, original code is a bug. Is it more complicated than just changing the return type and handful of isolated call sites?<br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 11, 2017 at 8:22 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Jul 11, 2017 at 3:44 AM Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jul 10, 2017 at 7:15 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><div><div class="m_-745827289689202173m_3031435710983279140m_200455747952102589m_3418869084197343960h5"><div dir="ltr">On Wed, Jul 5, 2017 at 8:02 AM Alexander Kornienko via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: alexfh<br>
Date: Tue Jul 4 08:13:02 2017<br>
New Revision: 307085<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=307085&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=307085&view=rev</a><br>
Log:<br>
Fix dangling StringRefs found by clang-tidy misc-dangling-handle check.<br>
<br>
Modified:<br>
llvm/trunk/tools/llvm-lto/<wbr>llvm-lto.cpp<br>
llvm/trunk/tools/llvm-readobj/<wbr>COFFDumper.cpp<br>
<br>
Modified: llvm/trunk/tools/llvm-lto/<wbr>llvm-lto.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-lto/llvm-lto.cpp?rev=307085&r1=307084&r2=307085&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/tools/llvm-<wbr>lto/llvm-lto.cpp?rev=307085&<wbr>r1=307084&r2=307085&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/tools/llvm-lto/<wbr>llvm-lto.cpp (original)<br>
+++ llvm/trunk/tools/llvm-lto/<wbr>llvm-lto.cpp Tue Jul 4 08:13:02 2017<br>
@@ -383,7 +383,7 @@ loadAllFilesForIndex(const ModuleSummary<br>
<br>
for (auto &ModPath : Index.modulePaths()) {<br>
const auto &Filename = ModPath.first();<br>
- auto CurrentActivity = "loading file '" + Filename + "'";<br>
+ std::string CurrentActivity = ("loading file '" + Filename + "'").str();<br>
auto InputOrErr = MemoryBuffer::getFile(<wbr>Filename);<br>
error(InputOrErr, "error " + CurrentActivity);<br>
InputBuffers.push_back(std::<wbr>move(*InputOrErr));<br>
@@ -475,7 +475,7 @@ private:<br>
std::vector<std::unique_ptr<<wbr>MemoryBuffer>> InputBuffers;<br>
for (unsigned i = 0; i < InputFilenames.size(); ++i) {<br>
auto &Filename = InputFilenames[i];<br>
- StringRef CurrentActivity = "loading file '" + Filename + "'";<br>
+ std::string CurrentActivity = "loading file '" + Filename + "'";<br>
auto InputOrErr = MemoryBuffer::getFile(<wbr>Filename);<br>
error(InputOrErr, "error " + CurrentActivity);<br>
InputBuffers.push_back(std::<wbr>move(*InputOrErr));<br>
@@ -710,7 +710,7 @@ private:<br>
std::vector<std::unique_ptr<<wbr>MemoryBuffer>> InputBuffers;<br>
for (unsigned i = 0; i < InputFilenames.size(); ++i) {<br>
auto &Filename = InputFilenames[i];<br>
- StringRef CurrentActivity = "loading file '" + Filename + "'";<br>
+ std::string CurrentActivity = "loading file '" + Filename + "'";<br>
auto InputOrErr = MemoryBuffer::getFile(<wbr>Filename);<br>
error(InputOrErr, "error " + CurrentActivity);<br>
InputBuffers.push_back(std::<wbr>move(*InputOrErr));<br>
<br>
Modified: llvm/trunk/tools/llvm-readobj/<wbr>COFFDumper.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-readobj/COFFDumper.cpp?rev=307085&r1=307084&r2=307085&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/tools/llvm-<wbr>readobj/COFFDumper.cpp?rev=<wbr>307085&r1=307084&r2=307085&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/tools/llvm-readobj/<wbr>COFFDumper.cpp (original)<br>
+++ llvm/trunk/tools/llvm-readobj/<wbr>COFFDumper.cpp Tue Jul 4 08:13:02 2017<br>
@@ -1637,7 +1637,11 @@ static StringRef getBaseRelocTypeName(ui<br>
case COFF::IMAGE_REL_BASED_HIGHADJ: return "HIGHADJ";<br>
case COFF::IMAGE_REL_BASED_ARM_<wbr>MOV32T: return "ARM_MOV32(T)";<br>
case COFF::IMAGE_REL_BASED_DIR64: return "DIR64";<br>
- default: return "unknown (" + llvm::utostr(Type) + ")";<br>
+ default: {<br>
+ static std::string Result;<br>
+ Result = "unknown (" + llvm::utostr(Type) + ")";<br>
+ return Result;<br></blockquote></div></div><div><br>This looks concerning/wrong. Now two callers tot his API could interfere with each other (the returned StringRef is only valid until the function is called again)<br><br>Perhaps this function should return std::string instead.<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>The motivation for this solution is to keep the cheap path cheap (and the expensive one isn't going to happen under normal circumstances anyway).</div></div></div></div></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>This is in readobj, and retrieving the string representation of something, presumably to show to a user - so I'm not sure the performance is that sensitive.<br><br>I think it's probably best to err on the side of unsurprising API until there's a need to performance tune this to that degree. Put another way: This doesn't seem like the way this code should've been written in the first place. I'd only expect to see subtle code like this if it were particularly strongly motivated by performance data, etc. (& even then, probably would use a local std::string in the caller, passed in by reference to be populated on the side when needed - I'd expect default construction of a std::string to be cheap enough to not interfere with perf requirements)<br> </div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> I've inspected the only caller, and it's not going to cause such issues. However, I should have probably added a FIXME about this or somehow draw the attention of someone who cares about this code to fix it properly.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
}<br>
}<br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></span></div></div>
</blockquote></div></div></div></blockquote></div></div></blockquote></div>
</blockquote></div>
</div></div></blockquote></div><br></div>