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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:373
+  if (File.empty())
+    WithColor::warning(errs(), ToolName) << Message << ".\n";
+  else
----------------
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.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:381
 LLVM_ATTRIBUTE_NORETURN void report_error(StringRef File, Twine Message) {
-  WithColor::error(errs(), ToolName)
-      << "'" << File << "': " << Message << ".\n";
+  if (File.empty())
+    WithColor::error(errs(), ToolName) << Message << ".\n";
----------------
grimar wrote:
> MaskRay wrote:
> > Keeping a separate `LLVM_ATTRIBUTE_NORETURN void report_error(Twine Message);` overload for the error message without a filename might be clearer. What do you think?
> I had to add this code to `report_error` and `report_warn` because of 2 users:
> 
> ```
>     report_warn("section '" + S +
>                     "' mentioned in a -j/--section option, but not "
>                     "found in any input file",
>                 "" /*FileName*/);
> ```
> 
> and
> 
> ```
> report_error("" /*FileName*/, "start address should be less than stop address");
> ```
> 
> I.e. these are kind of exceptions, as others calls always have `FileName`.
> 
> What I afraid here is that people might want to continue using the `void report_error(Twine Message)`
> everywhere if we have one. I am not sure what is better. Lets see what others think may be.
> 
> May be we should print something like the following for these two ?
> 
> ```
> warn: -j/--section: section XXX mentioned, but not found in any input file
> 
> error: --start-address/--stop-address: start address should be less than stop address
> ```
> 
> Then we do not need to have any code for an empty file name.
I like the idea of using the switch name as the "file name", but a couple of other options are:

1. Use "<cmdline>" or similar as the file name.
2. Call the no-filename overload `reportCmdLineError` or similar.


================
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);
 
----------------
I'm not entirely convinced that a filename here makes sense?


================
Comment at: tools/llvm-objdump/llvm-objdump.h:138
              StringRef ArchitectureName = StringRef());
+void report_warn(Twine Message, StringRef File);
 
----------------
report_warn -> report_warning (or even reportWarning to conform to LLVM style).


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

https://reviews.llvm.org/D66418