[llvm-commits] [PATCH] elfobjectwriter patch (ARM/MC/ELF)

[Jason Kim]

On Thu, Nov 11, 2010 at 10:11 AM, Renato Golin <Renato.Golin at arm.com> wrote:
> On 10/11/10 17:57, Jason Kim wrote:
>> tryA: move the functionality of the ELFObjectWriterImpl class into
>> ELFObjectWriter, and subclass ELFObjectWriter to
>> <ARCH>ELFObjectWriter.
>  >
> (...)
>>
>> tryB: subclass ELFObjectWriterImpl instead - I am still working out
>> the details on this one - but as of right now, it is just as complex
>> as the tryA case.
>
> I'm not an expert in MC, but I can't see anything wrong with neither
> approaches. The second one seems less invasive, but has the problems you
> mentioned.
>
> As MachO and ELF are very different, I can't see now the big problem of
> conceptually separating them.
>
> As far as I could gather, you only moved things around, so no problems
> for ARM. In this sense, I'm happy with both patches.


Hi Everyone,

tryB *is* less invasive, but tryA1 (included) is still the better approach IMO

tryA1 has 1 new class total, and two fewer slots, and no trampolines
(a plus IMO), and requires 4 changes to the places where
ELFObjectWriter is created, but given that this is already in a
arch-specific factory function, its not a big deal.

tryB has 2 new classes total, and has the trampolines (a PLUS if you
think ELF should be implemented in a similar fashion to the MachO
code)

I'd like to push in tryA - as I need this refactoring to proceed
further in ARM/MC/ELF work.

combined.patch is just a straight cat of all subpatches  in sequence,
so its larger than the svn diff (
arm-mc-elf-s07-elfwriter-tryA-combined.patch)

http://codereview.chromium.org/4828001
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm-mc-elf-s07-elfwriter-tryA-combined.patch
Type: text/x-patch
Size: 43386 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101111/7d10c638/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: combined.patch
Type: text/x-patch
Size: 81122 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101111/7d10c638/attachment-0001.bin>