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

[David Blaikie via llvm-commits]

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

> On Mon, Feb 1, 2016 at 6:48 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> 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).
>>
>
> You mentioned about declaring a couple of references because the value may
> be a large value, right?
>

Or if mutation is desired.

If copying them is not a problem, you can avoid Clang's warning by using
value:

for (std::pair<uintX_t, size_t> L : MipsLocalGotPos)

That way it's clear you're intentionally making a copy - but anything that
mentions the types can still potentially be incorrect & cause implicit
conversions to occur (be it in the pair (which has the bonus subtlety of
the const) or in individual variables).

I think in this case Clang doesn't warn, but would have to check.

I'm probably more auto-happy than most, and I wouldn't espouse that as the
One True Way or anything. You've certainly a few choices that are probably
all reasonable.


> In this case the values are both 64 or 32 bit integers, so I'd write
>
>   for (const auto &L : MipsLocalGotPos) {
>     uintX_t GotValue = L.first;
>     size_t GotIndex = L.second;
>     uint8_t *Entry = Buf + GotIndex* sizeof(uintX_t);
>     write<uintX_t, ELFT::TargetEndianness, sizeof(uintX_t)>(Entry,
> GotValue);
>   }
>
> instead of
>
>   for (const auto &L : MipsLocalGotPos) {
>     uintX_t &GotValue = L.first; // <-- has &
>     size_t &GotIndex = L.second;
>     uint8_t *Entry = Buf + L.second * sizeof(uintX_t);
>     write<uintX_t, ELFT::TargetEndianness, sizeof(uintX_t)>(Entry,
> L.first);
>   }
>
>
>>
>> - 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/053865fa/attachment.html>