[PATCH] D13055: [ELF2] Handle -m option

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Config.h:35
@@ +34,3 @@
+
+  ELFKind ElfKind = ELF64LEKind;
+  uint16_t EMachine = llvm::ELF::EM_X86_64;
----------------
denis-protivensky wrote:
> ruiu wrote:
> > denis-protivensky wrote:
> > > rafael wrote:
> > > > Can these two variables be left uninitialized? If not, please make them an explicit Optional<>. We really should not give EM_X86_64 or ELF64LE any special treatment.
> > > Will make ElfKind optional and set EMachine to EM_NONE.
> > Or you could define a new enum, ELFNoneKind and use the value as an indicator of absence of a valid value.
> Better to have the optional, since defining one enum value used in only one case as a default looks like bad design.
No, it's not always bad, but sometimes good. Absence of a valid value is often represented using a null pointer, for example.

================
Comment at: ELF/Driver.cpp:67
@@ -48,2 +66,3 @@
 
-static std::unique_ptr<InputFile> createFile(MemoryBufferRef MB) {
+static uint16_t getElfMachineType(StringRef Object) {
+  using namespace object;
----------------
denis-protivensky wrote:
> denis-protivensky wrote:
> > ruiu wrote:
> > > Passing an in-memory object as a StringRef feels a bit strange. Maybe you want to passs MemoryBuffer instead.
> > Will try.
> `MemoryBufferRef` doesn't have suitable methods to operate on bytes, so I'll in any case need to construct `StringRef` from `MemoryBufferRef` inside. No profit in changing the signature, especially for non-public method.
Why I felt that was a bit weird is because when I see a StringRef, I expect it would refer to some text string and not to arbitrary byte sequence (if that's the case, ArrayRef<uint8_t> would be suitable). So I'd think we should change the signature. You can call getBuffer() *inside* this function and not before calling this function.


http://reviews.llvm.org/D13055