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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
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";
----------------
jhenderson wrote:
> 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.
I like the idea of `reportCmdLineError`. I think it is almost the same as having `report_error(Twine Message)`, but
kills my consern about misusing the error reporting API. I am going to use it.


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

https://reviews.llvm.org/D66418