[llvm] r307085 - Fix dangling StringRefs found by clang-tidy misc-dangling-handle check.

[David Blaikie via llvm-commits]

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/b500aacd/attachment.html>