[llvm-commits] ELFReader.cpp update - Add support for References.

[Sid Manning]

On 09/14/12 17:15, Michael Spencer wrote:
> On Fri, Sep 14, 2012 at 2:15 PM, Sid Manning<sidneym at codeaurora.org>  wrote:
>>
...

>>
>> +  typedef ELFRelocationIterator<const Elf_Rela *>  Elf_Rela_Iter;
>> +  typedef ELFRelocationIterator<const Elf_Rel *>  Elf_Rel_Iter;
>
> These shouldn't have the const and *. ELFRelocationIterator already adds them.
> With them opeartor * actually returns const RelocT * const&. This is why you
> needed the reinterpret_casts in the other code.
>

Updated in this patch set.

...

>> +
>> +///
>> +/// Relocation References: Defined Atoms may contain
>> +/// references that will need to be patched before
>> +/// the executable is written.
>> +///
>
> This should match the style used elsewhere. No extra /// at the top and bottom
> and it should have a \brief, which in this case would be all the text that is
> currently there.
>

OK, changed all these to, /// \brief


...

>
>> +  virtual uint64_t targetSymbolIndex() const {
>> +    return _targetSymbolIndex;
>> +  }
>
> This shouldn't be virtual.
>

OK updated.

>> +
>> +  virtual Addend addend() const {
>> +    return _addend;
>> +  }
>> +
>> +  virtual void setAddend(Addend A) {
>> +    _addend = A;
>> +  }
>> +
>> +  virtual void setTarget(const Atom *newAtom) {
>> +    _target = newAtom;
>> +  }
>
> All of these virtual functions should be marked as final.
>
If all the functions are to be marked final then why not just mark the 
class as final, "class ELFReference final : public Reference", I marked 
all of these final.  What criteria is used in deciding when to mark a 
function final?  It seems like many of the Atom functions could be 
marked as final too.


...

>> +
>> +        auto&Ref = _relocationAddendRefences[sectionName];
>> +        for (; rai != rae; rai++) {
>> +          Ref.push_back(reinterpret_cast<const Elf_Rela *>(&*rai));
>
> This cast is incorrect and should be removed. See my comments from ELF.h.
>
Done.

>
> - Michael Spencer


Thanks,

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ELF.h.diff
Type: text/x-patch
Size: 4146 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120914/b9033636/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ReaderELF.cpp.diff
Type: text/x-patch
Size: 31576 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120914/b9033636/attachment-0001.bin>