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

[Jason Kim]

On Thu, Nov 11, 2010 at 2:21 PM, Jason Kim <jasonwkim at google.com> wrote:
> 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
tryA1 oops.

> 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
>