[PATCH] D66418: [llvm-objdump] - Cleanup the error reporting.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 20 02:53:39 PDT 2019
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
More information about the llvm-commits
mailing list