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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Config.h:26
@@ -20,1 +25,3 @@
 struct Configuration {
+  bool is64Bits() const {
+    return *ElfKind == ELF64BEKind || *ElfKind == ELF64LEKind;
----------------
We name this function is64() in COFF.

================
Comment at: ELF/Driver.cpp:65-66
@@ +64,4 @@
+  Config->ElfKind = ElfKind;
+  Config->EMachine = EMachine;
+}
+
----------------
I still think that inlining this function would be better.

================
Comment at: ELF/Driver.cpp:70
@@ +69,3 @@
+static std::unique_ptr<InputFile> createELFFile(MemoryBufferRef MB,
+                                                const char *&TargetSource) {
+  std::unique_ptr<ELFFileBase> File = createELFFile<T>(MB);
----------------
Add a new member variable to Config for TargetSource and remove this from this parameter list. I'd name Config->FirstObjName.

================
Comment at: ELF/Driver.cpp:82
@@ +81,3 @@
+
+  if (ElfKind != *Config->ElfKind || EMachine != Config->EMachine)
+    error(Twine(MB.getBufferIdentifier() + " is incompatible with " +
----------------
*Config->ElfKind doesn't look beautiful. I'd add ELFNoneKind enum and use that to represent the absence of a valid value.

================
Comment at: ELF/Driver.cpp:125-143
@@ +124,21 @@
+
+  struct Emulation {
+    const char *Name;
+    ELFKind ElfKind;
+    uint16_t EMachine;
+  };
+  static const Emulation Emulations[] = {
+      {"elf_i386", ELF32LEKind, EM_386},
+      {"elf_x86_64", ELF64LEKind, EM_X86_64},
+      {"elf32ppc", ELF32BEKind, EM_PPC},
+      {"elf64ppc", ELF64BEKind, EM_PPC64},
+  };
+
+  for (auto E : Emulations) {
+    if (Emul == E.Name) {
+      configureTarget(E.ElfKind, E.EMachine);
+      return;
+    }
+  }
+  error(Twine("Unknown emulation: ") + Emul);
+}
----------------
This looks more intricate than necessary. Plain if-then-else would be better.

  if (Emul == "elf_i386") {
    ...
    return
  }
  if (Emul == ...) {
  }
  error("unknown emulation name: "...)

================
Comment at: ELF/Driver.cpp:187
@@ -125,1 +186,3 @@
 
+  // Handle -m emulation
+  const char *TargetSource = "target architecture";
----------------
We don't have a comment for other options above.


http://reviews.llvm.org/D13055