[lld] r222455 - [mach-o] use reference with "auto" to prevent copies

[David Blaikie]

On Thu, Nov 20, 2014 at 2:39 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Thu, Nov 20, 2014 at 1:46 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Thu, Nov 20, 2014 at 1:19 PM, Nick Kledzik <kledzik at apple.com> wrote:
>>
>>> Author: kledzik
>>> Date: Thu Nov 20 15:19:58 2014
>>> New Revision: 222455
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=222455&view=rev
>>> Log:
>>> [mach-o] use reference with "auto" to prevent copies
>>>
>>> Patch by Jean-Daniel Dupas
>>>
>>> Modified:
>>>     lld/trunk/lib/ReaderWriter/MachO/File.h
>>>
>>> Modified: lld/trunk/lib/ReaderWriter/MachO/File.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/MachO/File.h?rev=222455&r1=222454&r2=222455&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/lib/ReaderWriter/MachO/File.h (original)
>>> +++ lld/trunk/lib/ReaderWriter/MachO/File.h Thu Nov 20 15:19:58 2014
>>> @@ -118,13 +118,13 @@ public:
>>>    MachODefinedAtom *findAtomCoveringAddress(const Section &section,
>>>                                              uint64_t offsetInSect,
>>>                                              uint32_t
>>> *foundOffsetAtom=nullptr) {
>>> -    auto pos = _sectionAtoms.find(&section);
>>> +    const auto& pos = _sectionAtoms.find(&section);
>>>
>>
>> This one's a bit weird - 'find' (for any standard-like container, at
>> least) returns an iterator by value. While taking a reference to that is
>> valid (reference lifetime extension & all that) it's a bit surprising &
>> unnecessary (has basically the same semantics as the non-reference code).
>>
>>
>>>      if (pos == _sectionAtoms.end())
>>>        return nullptr;
>>> -    auto vec = pos->second;
>>> +    const auto& vec = pos->second;
>>>      assert(offsetInSect < section.content.size());
>>>      // Vector of atoms for section are already sorted, so do binary
>>> search.
>>> -    auto atomPos = std::lower_bound(vec.begin(), vec.end(),
>>> offsetInSect,
>>> +    const auto& atomPos = std::lower_bound(vec.begin(), vec.end(),
>>> offsetInSect,
>>>
>>
>> Same here, lower_bound just returns an iterator by value, this doesn't
>> look like it's avoiding any copies.
>>
>
> Also, wouldn't this create a reference to a temporary?
>

Right - but reference extension semantics mean this isn't actually
incorrect/unsafe...


>
> -- Sean Silva
>
>
>>
>>
>>>          [offsetInSect](const SectionOffsetAndAtom &ao,
>>>                         uint64_t targetAddr) -> bool {
>>>            // Each atom has a start offset of its slice of the
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141120/8400175a/attachment.html>