<div dir="ltr">Just committed the fix as r309092.</div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Jul 22, 2017 at 1:30 AM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Did you ever get around to this? I didn't see a follow up.<br>
<div class="HOEnZb"><div class="h5"><br>
Alexander Kornienko via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
> I can change the return type to std::string tomorrow, if nobody does that<br>
> first.<br>
><br>
> On Tue, Jul 11, 2017 at 5:26 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
>> Nah, there's only one caller by the looks of it, and it looks like it<br>
>> doesn't need updating if this function changes from StringRef->std::string.<br>
>><br>
>> On Tue, Jul 11, 2017 at 8:25 AM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
>><br>
>>> Also agree, original code is a bug. Is it more complicated than just<br>
>>> changing the return type and handful of isolated call sites?<br>
>>> On Tue, Jul 11, 2017 at 8:22 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>>> On Tue, Jul 11, 2017 at 3:44 AM Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>><br>
>>>> wrote:<br>
>>>><br>
>>>>> On Mon, Jul 10, 2017 at 7:15 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>>>>> wrote:<br>
>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> On Wed, Jul 5, 2017 at 8:02 AM Alexander Kornienko via llvm-commits <<br>
>>>>>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>>>>>><br>
>>>>>>> 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<br>
>>>>>>> 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-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/tools/llvm-</a><br>
>>>>>>> lto/llvm-lto.cpp?rev=307085&<wbr>r1=307084&r2=307085&view=diff<br>
>>>>>>> ==============================<wbr>==============================<br>
>>>>>>> ==================<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 +<br>
>>>>>>> "'").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>
>>>>>>> "'";<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>
>>>>>>> "'";<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-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/tools/llvm-</a><br>
>>>>>>> readobj/COFFDumper.cpp?rev=<wbr>307085&r1=307084&r2=307085&<wbr>view=diff<br>
>>>>>>> ==============================<wbr>==============================<br>
>>>>>>> ==================<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<br>
>>>>>>> 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>
>>>>>>><br>
>>>>>><br>
>>>>>> This looks concerning/wrong. Now two callers tot his API could<br>
>>>>>> interfere with each other (the returned StringRef is only valid until the<br>
>>>>>> function is called again)<br>
>>>>>><br>
>>>>>> Perhaps this function should return std::string instead.<br>
>>>>>><br>
>>>>><br>
>>>>> The motivation for this solution is to keep the cheap path cheap (and<br>
>>>>> the expensive one isn't going to happen under normal circumstances anyway).<br>
>>>>><br>
>>>><br>
>>>> This is in readobj, and retrieving the string representation of<br>
>>>> something, presumably to show to a user - so I'm not sure the performance<br>
>>>> is that sensitive.<br>
>>>><br>
>>>> I think it's probably best to err on the side of unsurprising API until<br>
>>>> there's a need to performance tune this to that degree. Put another way:<br>
>>>> This doesn't seem like the way this code should've been written in the<br>
>>>> first place. I'd only expect to see subtle code like this if it were<br>
>>>> particularly strongly motivated by performance data, etc. (& even then,<br>
>>>> probably would use a local std::string in the caller, passed in by<br>
>>>> reference to be populated on the side when needed - I'd expect default<br>
>>>> construction of a std::string to be cheap enough to not interfere with perf<br>
>>>> requirements)<br>
>>>><br>
>>>><br>
>>>>> I've inspected the only caller, and it's not going to cause such<br>
>>>>> issues. However, I should have probably added a FIXME about this or somehow<br>
>>>>> draw the attention of someone who cares about this code to fix it properly.<br>
>>>>><br>
>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>>> + }<br>
>>>>>>> }<br>
>>>>>>> }<br>
>>>>>>><br>
>>>>>>><br>
>>>>>>><br>
>>>>>>> ______________________________<wbr>_________________<br>
>>>>>>> llvm-commits mailing list<br>
>>>>>>> <a href="mailto:llvm-commits@lists.llvm.org">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>
>>>>>>><br>
>>>>>><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">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>
</div></div></blockquote></div><br></div>