This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] - Cleanup the error reporting.
ClosedPublic

Authored by grimar on Aug 19 2019, 7:14 AM.

Details

Summary

The error reporting function are not consistent.

Before this change:

  • They had inconsistent naming (e.g. 'error' vs 'report_error').
  • Some of them reported the object name, others - dont.
  • Some of them accepted the case when there was no error. (i.e. error code or Error had a success value).

This patch tries to cleanup it a bit.

It also renames report_error -> reportError, report_warning -> reportWarning
and removes a full stop from messages.

Diff Detail

Event Timeline

grimar created this revision.Aug 19 2019, 7:14 AM
grimar marked an inline comment as done.Aug 19 2019, 7:36 AM
grimar added inline comments.
tools/llvm-objdump/llvm-objdump.h
130

btw, to be consistent, this should be void report_error(Twine Message, StringRef File)
But I'd like to do this in a separate NFC commit.

grimar updated this revision to Diff 215901.EditedAug 19 2019, 7:55 AM
grimar edited the summary of this revision. (Show Details)
  • Rebased on top.
MaskRay accepted this revision.Aug 19 2019, 9:33 PM
MaskRay added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
381

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?

447

/*FileName=*/""

See above. Keeping a separate LLVM_ATTRIBUTE_NORETURN void report_error(Twine Message); might be better.

450
- static const Target *getTarget(const ObjectFile *Obj = nullptr) {
+ static const Target *getTarget(const ObjectFile *Obj) {

Nice!

2220

/*FileName=*/"" is probably more common. (If you add a space, clang-format will delete it for you.)

See above. Keeping a separate LLVM_ATTRIBUTE_NORETURN void report_error(Twine Message); might be better.

This revision is now accepted and ready to land.Aug 19 2019, 9:33 PM
grimar marked an inline comment as done.Aug 20 2019, 1:41 AM
grimar added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
381

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.

jhenderson added inline comments.Aug 20 2019, 2:53 AM
tools/llvm-objdump/llvm-objdump.cpp
373

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.

381

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.
1576–1577

I'm not entirely convinced that a filename here makes sense?

tools/llvm-objdump/llvm-objdump.h
138

report_warn -> report_warning (or even reportWarning to conform to LLVM style).

grimar marked an inline comment as done.Aug 20 2019, 6:05 AM
grimar added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
381

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.

grimar updated this revision to Diff 216163.Aug 20 2019, 8:37 AM
grimar marked 7 inline comments as done.
  • Address review comments.
tools/llvm-objdump/llvm-objdump.cpp
373

Ok, I wasn't sure here. Removed the full stop.

1576–1577

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?).

tools/llvm-objdump/llvm-objdump.h
138

Let me stick with report_warning in this patch. (To be a bit more consistent with report_error for now).

grimar updated this revision to Diff 216165.Aug 20 2019, 8:41 AM
  • Cosmetic change.
grimar planned changes to this revision.Aug 20 2019, 8:42 AM

I'll recheck it.

grimar updated this revision to Diff 216173.Aug 20 2019, 9:12 AM

Ready for review.

This revision is now accepted and ready to land.Aug 20 2019, 9:12 AM

We probably should do reportWarning to conform to LLVM's coding style since it is a new function.

The description should mention that full stops are now deleted from the messages.

+1 to changing report_error->reportError, same for reportWarning. I suppose it can be a followup NFC patch since it's technically consistent with the existing code. LGTM to the rest.

grimar updated this revision to Diff 216341.Aug 21 2019, 1:02 AM
grimar edited the summary of this revision. (Show Details)

Ok. I wanted to rename in a followup, but it is not a problem to do now.

Changes:

  • report_error -> reportError
  • report_warning -> reportWarning
MaskRay accepted this revision.Aug 21 2019, 1:06 AM
jhenderson added inline comments.Aug 21 2019, 2:29 AM
tools/llvm-objdump/llvm-objdump.cpp
422

LLVM_ATTRIBUTE_NORETURN?

grimar marked an inline comment as done.Aug 21 2019, 2:35 AM
tools/llvm-objdump/llvm-objdump.cpp
422

Seems - yes. Is anything else needed to be fixed or will you be OK if I land this patch with this fix?

jhenderson accepted this revision.Aug 21 2019, 3:50 AM

LGTM, with my suggestion.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 4:07 AM