[llvm] r307085 - Fix dangling StringRefs found by clang-tidy misc-dangling-handle check.
Alexander Kornienko via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 11 08:35:08 PDT 2017
I can change the return type to std::string tomorrow, if nobody does that
first.
On Tue, Jul 11, 2017 at 5:26 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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/e4d747cb/attachment.html>
More information about the llvm-commits
mailing list