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

[Justin Bogner via llvm-commits]

Did you ever get around to this? I didn't see a follow up.

Alexander Kornienko via llvm-commits <llvm-commits at lists.llvm.org> writes:
> 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
>>>>>>>
>>>>>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits