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

[Rui Ueyama via llvm-commits]

ruiu added a subscriber: ruiu.

================
Comment at: ELF/Driver.cpp:44
@@ +43,3 @@
+  const char *Name;
+  ELFKind ElfKind;
+  uint16_t EMachine;
----------------
ElfKind -> ELFKind

================
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;
----------------
Passing an in-memory object as a StringRef feels a bit strange. Maybe you want to passs MemoryBuffer instead.

================
Comment at: ELF/Driver.cpp:76-77
@@ +75,4 @@
+    return ((const Elf_Ehdr_Impl<ELF32BE> *)Object.data())->e_machine;
+  else
+    return ((const Elf_Ehdr_Impl<ELF32LE> *)Object.data())->e_machine;
+}
----------------
No need for `else` because the other path ends with return.

================
Comment at: ELF/Driver.cpp:81-82
@@ +80,4 @@
+static inline void configureTarget(ELFKind ElfKind, uint16_t EMachine) {
+  Config->ElfKind = ElfKind;
+  Config->EMachine = EMachine;
+}
----------------
This function is probably too short to be defined as a function. Just write two lines of code where you call this function.

================
Comment at: ELF/Driver.cpp:96
@@ +95,3 @@
+    configureTarget(ElfKind, EMachine);
+    TargetDefined = true;
+    TargetSource = MB.getBufferIdentifier().data();
----------------
I'd remove this TargetDefined variable. Instead, I'd change the type of Config->ELFKind to Optional<>, and check for the existence of the value instead of a separate flag.

================
Comment at: ELF/Driver.cpp:103
@@ +102,3 @@
+    error(Twine(MB.getBufferIdentifier() + " is incompatible with " +
+                TargetSource));
+  return std::move(File);
----------------
Looks like TargetSource is used just for printing this error message. Can you remove that? I think a detailed error message is generally good, but this error message is used rarely, and probably that wouldn't match the cost of having another variable here. Then we can keep createELFFile to take only one argument just like before.

================
Comment at: ELF/Driver.cpp:114
@@ -54,3 +113,3 @@
     return make_unique<ArchiveFile>(MB);
 
   if (Magic == file_magic::elf_shared_object)
----------------
Remove this

================
Comment at: ELF/Driver.cpp:116
@@ -56,3 +115,3 @@
   if (Magic == file_magic::elf_shared_object)
-    return createELFFile<SharedFile>(MB);
+    return createTargetELFFile<SharedFile>(MB, TargetDefined, TargetSource);
 
----------------
Let's keep the original function name.

================
Comment at: ELF/Driver.cpp:117
@@ -58,1 +116,3 @@
+    return createTargetELFFile<SharedFile>(MB, TargetDefined, TargetSource);
 
+  return createTargetELFFile<ObjectFile>(MB, TargetDefined, TargetSource);
----------------
and this blank lines.

================
Comment at: ELF/Driver.cpp:121
@@ +120,3 @@
+
+static void applyEmulation(StringRef Emul) {
+  for (auto E : Emulations) {
----------------
Move the definition of Emulations inside here as a static local constant because only this function uses that.


http://reviews.llvm.org/D13055