[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:
> 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.

================
Comment at: ELF/Driver.cpp:80
@@ +79,3 @@
+
+static inline void configureTarget(ELFKind ElfKind, uint16_t EMachine) {
+  Config->ElfKind = ElfKind;
----------------
denis-protivensky wrote:
> rafael wrote:
> > Drop the inline.
> Is this something style-guide related or error-prone? Why to drop it?
The rule of thumb is we shouldn't write `inline` unless it's proved to be necessary. Basically this piece of code doesn't have to be optimized, or even the entire Driver doesn't have to be fast, because the runtime cost of Driver is negligible after all. I wouldn't even think about speed when writing the Driver to save my brain power to focus on something more important. :)

================
Comment at: ELF/Driver.cpp:81-82
@@ +80,4 @@
+static inline void configureTarget(ELFKind ElfKind, uint16_t EMachine) {
+  Config->ElfKind = ElfKind;
+  Config->EMachine = EMachine;
+}
----------------
denis-protivensky wrote:
> ruiu wrote:
> > This function is probably too short to be defined as a function. Just write two lines of code where you call this function.
> I'm against the code duplication in general. Even for two-liner, so I'd like to leave it here.
Well, that depends. If they are just one-line assignment statement to a Config member variable, you wouldn't make the "duplicate" assignments to a function. Two assignments are still more readable than a function call.

================
Comment at: ELF/Driver.cpp:103
@@ +102,3 @@
+    error(Twine(MB.getBufferIdentifier() + " is incompatible with " +
+                TargetSource));
+  return std::move(File);
----------------
denis-protivensky wrote:
> ruiu wrote:
> > 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.
> That was my original design, but @rafael insisted we need this to avoid test regression. See comments for `incompatible.s` test below.
I feel like there must be some way to remove this parameter. Can we do this check not in this function but at Driver.cpp line 189?


http://reviews.llvm.org/D13055