[PATCH] D24060: [lld][ELF] Add support for -b binary

[Rui Ueyama via llvm-commits]

Creater sounds better, but I don't find "Simple" is a meaningful prefix.
Can you name ELFCreater?

On Thu, Sep 8, 2016 at 2:00 PM, Michael Spencer <bigcheesegs at gmail.com>
wrote:

> On Thu, Sep 8, 2016 at 1:47 PM, Rui Ueyama <ruiu at google.com> wrote:
>
>> It reads a binary file and wrap it with ELF header/trailer. And overall
>> it is part of the reader rather than the writer from the point of view of
>> the entire linking process. So I at least want to avoid "Writer".
>>
>
> SimpleELFWriter doesn't read anything. It writes ELF files. If we want to
> avoid writer I'm fine with SimpleELFCreator.
>
> - Michael Spencer
>
>
>>
>> On Thu, Sep 8, 2016 at 1:38 PM, Michael Spencer <bigcheesegs at gmail.com>
>> wrote:
>>
>>> On Thu, Sep 8, 2016 at 1:27 PM, Rui Ueyama <ruiu at google.com> wrote:
>>>
>>>> Doesn't BinaryFile make sense? We have InputFiles to handle input
>>>> files, so BinaryFile sounds like it is to handle binary file inputs. Do you
>>>> have other suggestions?
>>>>
>>>
>>> SimpleELFWriter isn't an input. It's a class that generates ELF files.
>>>
>>> - Michael Spencer
>>>
>>>
>>>>
>>>> On Thu, Sep 8, 2016 at 1:16 PM, Michael Spencer <bigcheesegs at gmail.com>
>>>> wrote:
>>>>
>>>>> On Thu, Sep 8, 2016 at 9:19 AM, Rui Ueyama <ruiu at google.com> wrote:
>>>>>
>>>>>> ruiu added inline comments.
>>>>>>
>>>>>> ================
>>>>>> Comment at: ELF/CMakeLists.txt:20
>>>>>> @@ -19,2 +19,3 @@
>>>>>>    ScriptParser.cpp
>>>>>> +  SimpleELFWriter.cpp
>>>>>>    Strings.cpp
>>>>>> ----------------
>>>>>> I still don't like this file name. `SimpleELFWriter` sounds like it
>>>>>> is for -o binary instead of -b. Can you rename BinaryFile.cpp?
>>>>>>
>>>>>
>>>>> I'm fine with finding another name, but BinaryFile isn't at all what
>>>>> this class is.
>>>>>
>>>>> - Michael Spencer
>>>>>
>>>>>
>>>>>>
>>>>>> ================
>>>>>> Comment at: ELF/InputFiles.cpp:736
>>>>>> @@ +735,3 @@
>>>>>> +template <class ELFT> std::unique_ptr<InputFile>
>>>>>> BinaryFile::createELF() {
>>>>>> +  SimpleELFWriter<ELFT> ELF(ET_REL, Config->EMachine);
>>>>>> +  auto DataSec = ELF.addSection(".data");
>>>>>> ----------------
>>>>>> Please move these details to SimpleELFWriter.cpp.
>>>>>>
>>>>>>
>>>>>> https://reviews.llvm.org/D24060
>>>>>>
>>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160908/10605bce/attachment.html>