[lld] r259455 - Replace auto with the real type.

[David Blaikie via llvm-commits]

On Mon, Feb 1, 2016 at 6:45 PM, Rui Ueyama <ruiu at google.com> wrote:

> On Mon, Feb 1, 2016 at 6:36 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Mon, Feb 1, 2016 at 6:29 PM, Rui Ueyama via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: ruiu
>>> Date: Mon Feb  1 20:29:03 2016
>>> New Revision: 259455
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=259455&view=rev
>>> Log:
>>> Replace auto with the real type.
>>>
>>> Modified:
>>>     lld/trunk/ELF/OutputSections.cpp
>>>
>>> Modified: lld/trunk/ELF/OutputSections.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/OutputSections.cpp?rev=259455&r1=259454&r2=259455&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/ELF/OutputSections.cpp (original)
>>> +++ lld/trunk/ELF/OutputSections.cpp Mon Feb  1 20:29:03 2016
>>> @@ -144,7 +144,7 @@ template <class ELFT> void GotSection<EL
>>>
>>>  template <class ELFT> void GotSection<ELFT>::writeTo(uint8_t *Buf) {
>>>    Target->writeGotHeader(Buf);
>>> -  for (const auto &L : MipsLocalGotPos) {
>>> +  for (std::pair<uintX_t, size_t> &L : MipsLocalGotPos) {
>>>
>>
>> This is probably accidentally causing a copy, because the value type of a
>> map is usually std::pair<const K, V> - so this sort of for loop introduces
>> an implicit conversion and then causes local reference extension to occur
>> (Clang has a warning for this - so I hope this is triggering a warning)
>>
>> This is one of the reasons auto is particularly encouraged in places like
>> this - to avoid the possibility of mismatching with the type (though
>> Clang's warning makes this a less risky proposition)
>>
>
> Thank you for pointing that out. What's the best practice here? Just use
> auto and immediately assign both values in a pair to some variables (to
> give them names)?
>

If the name/use of the map is not sufficient for the code to make sense
(eg: if the map is a class member, but used infrequently enough that anyone
looking at the class would have a clear idea of the key and value) as-is,
I'm not sure there's any particular best practice.

If the type information is particularly informative - maybe leaving it
as-is (& fixing the const) might suffice, if the Clang warning is doing its
job (are you using Clang to build? You might need to rebuild your host
Clang compiler, maybe?).

But alternatively, yes, it's not entirely uncommon to declare a couple of
references to the key and value to give them informative names (and/or type
information).

- Dave


>
>
>>      uint8_t *Entry = Buf + L.second * sizeof(uintX_t);
>>>      write<uintX_t, ELFT::TargetEndianness, sizeof(uintX_t)>(Entry,
>>> L.first);
>>>    }
>>>
>>>
>>> _______________________________________________
>>> 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/20160201/a00aaa95/attachment.html>