[llvm] r307085 - Fix dangling StringRefs found by clang-tidy misc-dangling-handle check.
Alexander Kornienko via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 26 03:15:31 PDT 2017
Just committed the fix as r309092.
On Sat, Jul 22, 2017 at 1:30 AM, Justin Bogner <mail at justinbogner.com>
wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170726/f7f94682/attachment.html>
More information about the llvm-commits
mailing list