[llvm] r307085 - Fix dangling StringRefs found by clang-tidy misc-dangling-handle check.
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 11 08:26:04 PDT 2017
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.
On Tue, Jul 11, 2017 at 8:25 AM Zachary Turner <zturner at google.com> wrote:
> Also agree, original code is a bug. Is it more complicated than just
> changing the return type and handful of isolated call sites?
> On Tue, Jul 11, 2017 at 8:22 AM David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Tue, Jul 11, 2017 at 3:44 AM Alexander Kornienko <alexfh at google.com>
>> wrote:
>>
>>> On Mon, Jul 10, 2017 at 7:15 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Wed, Jul 5, 2017 at 8:02 AM Alexander Kornienko via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: alexfh
>>>>> Date: Tue Jul 4 08:13:02 2017
>>>>> New Revision: 307085
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=307085&view=rev
>>>>> Log:
>>>>> Fix dangling StringRefs found by clang-tidy misc-dangling-handle check.
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/tools/llvm-lto/llvm-lto.cpp
>>>>> llvm/trunk/tools/llvm-readobj/COFFDumper.cpp
>>>>>
>>>>> Modified: llvm/trunk/tools/llvm-lto/llvm-lto.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-lto/llvm-lto.cpp?rev=307085&r1=307084&r2=307085&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/tools/llvm-lto/llvm-lto.cpp (original)
>>>>> +++ llvm/trunk/tools/llvm-lto/llvm-lto.cpp Tue Jul 4 08:13:02 2017
>>>>> @@ -383,7 +383,7 @@ loadAllFilesForIndex(const ModuleSummary
>>>>>
>>>>> for (auto &ModPath : Index.modulePaths()) {
>>>>> const auto &Filename = ModPath.first();
>>>>> - auto CurrentActivity = "loading file '" + Filename + "'";
>>>>> + std::string CurrentActivity = ("loading file '" + Filename +
>>>>> "'").str();
>>>>> auto InputOrErr = MemoryBuffer::getFile(Filename);
>>>>> error(InputOrErr, "error " + CurrentActivity);
>>>>> InputBuffers.push_back(std::move(*InputOrErr));
>>>>> @@ -475,7 +475,7 @@ private:
>>>>> std::vector<std::unique_ptr<MemoryBuffer>> InputBuffers;
>>>>> for (unsigned i = 0; i < InputFilenames.size(); ++i) {
>>>>> auto &Filename = InputFilenames[i];
>>>>> - StringRef CurrentActivity = "loading file '" + Filename + "'";
>>>>> + std::string CurrentActivity = "loading file '" + Filename + "'";
>>>>> auto InputOrErr = MemoryBuffer::getFile(Filename);
>>>>> error(InputOrErr, "error " + CurrentActivity);
>>>>> InputBuffers.push_back(std::move(*InputOrErr));
>>>>> @@ -710,7 +710,7 @@ private:
>>>>> std::vector<std::unique_ptr<MemoryBuffer>> InputBuffers;
>>>>> for (unsigned i = 0; i < InputFilenames.size(); ++i) {
>>>>> auto &Filename = InputFilenames[i];
>>>>> - StringRef CurrentActivity = "loading file '" + Filename + "'";
>>>>> + std::string CurrentActivity = "loading file '" + Filename + "'";
>>>>> auto InputOrErr = MemoryBuffer::getFile(Filename);
>>>>> error(InputOrErr, "error " + CurrentActivity);
>>>>> InputBuffers.push_back(std::move(*InputOrErr));
>>>>>
>>>>> Modified: llvm/trunk/tools/llvm-readobj/COFFDumper.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-readobj/COFFDumper.cpp?rev=307085&r1=307084&r2=307085&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/tools/llvm-readobj/COFFDumper.cpp (original)
>>>>> +++ llvm/trunk/tools/llvm-readobj/COFFDumper.cpp Tue Jul 4 08:13:02
>>>>> 2017
>>>>> @@ -1637,7 +1637,11 @@ static StringRef getBaseRelocTypeName(ui
>>>>> case COFF::IMAGE_REL_BASED_HIGHADJ: return "HIGHADJ";
>>>>> case COFF::IMAGE_REL_BASED_ARM_MOV32T: return "ARM_MOV32(T)";
>>>>> case COFF::IMAGE_REL_BASED_DIR64: return "DIR64";
>>>>> - default: return "unknown (" + llvm::utostr(Type) + ")";
>>>>> + default: {
>>>>> + static std::string Result;
>>>>> + Result = "unknown (" + llvm::utostr(Type) + ")";
>>>>> + return Result;
>>>>>
>>>>
>>>> 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)
>>>>
>>>> Perhaps this function should return std::string instead.
>>>>
>>>
>>> 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).
>>>
>>
>> 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.
>>
>> 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)
>>
>>
>>> 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.
>>>
>>>
>>>>
>>>>
>>>>> + }
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170711/b6a2c710/attachment.html>
More information about the llvm-commits
mailing list