[PATCH] D57680: [llvm-objdump] Implement `-Mreg-names-raw`/`-std` options.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/MC/MCInstPrinter.h:68
+  /// Customize the printer according to a command line option.
+  /// The options are passed by `llvm-objdump` one by one.
+  /// Return False for an unrecognized option so that the tool
----------------
This comment should not mention llvm-objdump. It might be the only user now, but there's nothing stopping other users in the future.


================
Comment at: include/llvm/MC/MCInstPrinter.h:69
+  /// The options are passed by `llvm-objdump` one by one.
+  /// Return False for an unrecognized option so that the tool
+  /// can issue an error message.
----------------
False -> false. Use the @returns syntax too, so that it gets sensibly formatted by doxygen.


================
Comment at: lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp:83
+  } else
+    return false;
+}
----------------
grimar wrote:
> You can avoid using `else` after `return`:
> 
> ```
> if (Opt == "reg-names-std") {
>   DefaultAltIdx = ARM::NoRegAltName;
>   return true;
> } 
> if (Opt == "reg-names-raw") {
>   DefaultAltIdx = ARM::RegNamesRaw;
>   return true;
> } 
> return false;
> ```
There's still an `else if`  a few lines up here, which can just become a regular if, as @grimar proposed.


================
Comment at: test/tools/llvm-objdump/ARM/reg-names.s:7
+@ RUN:   | FileCheck -check-prefix=RAW %s
+
+.text
----------------
May be worth testing what happens if you specify both (i.e. show last-one-wins).


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1471
 
+  for (const std::string &Opt : DisassemblerOptions)
+    if (!IP->applyTargetSpecificCLOption(Opt))
----------------
`StringRef`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57680/new/

https://reviews.llvm.org/D57680