[PATCH] D66418: [llvm-objdump] - Cleanup the error reporting.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:373
+  if (File.empty())
+    WithColor::warning(errs(), ToolName) << Message << ".\n";
+  else
----------------
jhenderson wrote:
> I'd be more inclined to remove the full stop in the error messages than add it to the warnings. I don't believe we usually add full stops to diagnostic messages.
Ok, I wasn't sure here. Removed the full stop.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1576-1577
     if (!IP->applyTargetSpecificCLOption(Opt))
-      error("Unrecognized disassembler option: " + Opt);
+      report_error(Obj->getFileName(),
+                   "Unrecognized disassembler option: " + Opt);
 
----------------
jhenderson wrote:
> I'm not entirely convinced that a filename here makes sense?
Well. `applyTargetSpecificCLOption` is not the code I am familar with, but seems it
has different implementations for `ARMInstPrinter` and a generic case which is:

>   /// Customize the printer according to a command line option.
>   /// @return true if the option is recognized and applied.
>   virtual bool applyTargetSpecificCLOption(StringRef Opt) { return false; }

What makes me think that for the different objects we might have the different result.
(e.g. when we have objects of different types in archive. It's a corner case probably, but seems a valid one?).




================
Comment at: tools/llvm-objdump/llvm-objdump.h:138
              StringRef ArchitectureName = StringRef());
+void report_warn(Twine Message, StringRef File);
 
----------------
jhenderson wrote:
> report_warn -> report_warning (or even reportWarning to conform to LLVM style).
Let me stick with `report_warning` in this patch. (To be a bit more consistent with `report_error` for now).


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

https://reviews.llvm.org/D66418