[llvm] r244455 - Modify r244405 to clearer code, per David Blaikie suggestion.

[Yaron Keren via llvm-commits]

Ah, the bot failed just between the two revisions, that's to be expected.
I'll recommit.


2015-08-10 21:23 GMT+03:00 Yaron Keren <yaron.keren at gmail.com>:

> This actually failed in the bot, I reverted. Looking into it.
>
> 2015-08-10 21:19 GMT+03:00 David Blaikie <dblaikie at gmail.com>:
>
>> Thanks!
>
>
> On Mon, Aug 10, 2015 at 11:07 AM, Yaron Keren <yaron.keren at gmail.com>
> wrote:
>
>> r244470+ r244471, thanks!
>>
>>
>> 2015-08-10 20:30 GMT+03:00 David Blaikie <dblaikie at gmail.com>:
>>
>>>
>>>
>>> On Mon, Aug 10, 2015 at 9:56 AM, Yaron Keren <yaron.keren at gmail.com>
>>> wrote:
>>>
>>>> Oops, I didn't notice that additional change when editing and did not
>>>> copy & paste.
>>>> Fixed + added comment in r244461.
>>>>
>>>
>>> It's still pretty easy to roll the two expressions in together (the
>>> comment helps, for sure - though it's easy to gloss over them). Doing all
>>> the variable initialization on the local, rather than in the vector, would
>>> separate the local creation from the push_back a lot & make this probably
>>> fine without a comment, I think.
>>>
>>> - David
>>>
>>>
>>>>
>>>>
>>>> 2015-08-10 19:46 GMT+03:00 David Blaikie <dblaikie at gmail.com>:
>>>>
>>>>>
>>>>>
>>>>> On Mon, Aug 10, 2015 at 9:15 AM, Yaron Keren via llvm-commits <
>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> Author: yrnkrn
>>>>>> Date: Mon Aug 10 11:15:51 2015
>>>>>> New Revision: 244455
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=244455&view=rev
>>>>>> Log:
>>>>>> Modify r244405 to clearer code, per David Blaikie suggestion.
>>>>>>
>>>>>>
>>>>>> Modified:
>>>>>>     llvm/trunk/tools/dsymutil/DwarfLinker.cpp
>>>>>>
>>>>>> Modified: llvm/trunk/tools/dsymutil/DwarfLinker.cpp
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/dsymutil/DwarfLinker.cpp?rev=244455&r1=244454&r2=244455&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/tools/dsymutil/DwarfLinker.cpp (original)
>>>>>> +++ llvm/trunk/tools/dsymutil/DwarfLinker.cpp Mon Aug 10 11:15:51 2015
>>>>>> @@ -2884,8 +2884,8 @@ void DwarfLinker::patchLineTableForUnit(
>>>>>>        if (StopAddress != -1ULL && !Seq.empty()) {
>>>>>>          // Insert end sequence row with the computed end address, but
>>>>>>          // the same line as the previous one.
>>>>>> -        Seq.reserve(Seq.size() + 1);
>>>>>> -        Seq.emplace_back(Seq.back());
>>>>>> +        auto NextLine = Seq.back();
>>>>>> +        Seq.emplace_back(NextLine);
>>>>>>
>>>>>
>>>>> I don't think there's any benefit to emplace_back here since there are
>>>>> no explicit conversions required. I'd probably usse push_back - and this
>>>>> still seems like it'd be easy for someone to just decide to fold the two
>>>>> statements together & create the original bug. The suggestion I was making
>>>>> was to avoid doing the initialization (the following several assignment
>>>>> statements \/ ) on the container element itself, but do it on the local.
>>>>> Then push that local onto the sequence - that seems less subtle (or at
>>>>> least it hides it better by not making it look like a trivial cleanup to
>>>>> collapse two expressions would be an improvement).
>>>>>
>>>>>
>>>>>>          Seq.back().Address = StopAddress;
>>>>>>          Seq.back().EndSequence = 1;
>>>>>>          Seq.back().PrologueEnd = 0;
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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/20150810/b2e12235/attachment.html>