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

Jason Kim jasonwkim at google.com
Thu Nov 11 15:21:50 PST 2010


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

oops. I resent the older tryA patch by mistake :-(  Sorry for the noise

The differences between A and A1 are minimal - tryA1 is a rebased
version of tryA at -r118813
rior included combined.patch is the A1 sequence of sub-patches.
The diffview URL shows the correct rebased combined diff.

-jason

>>
>> http://codereview.chromium.org/4828001
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm-mc-elf-s07-tryA1.patch
Type: text/x-patch
Size: 46273 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101111/46514d93/attachment.bin>


More information about the llvm-commits mailing list