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

[Rui Ueyama via llvm-commits]

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? 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/3f65676c/attachment.html>