Page MenuHomePhabricator

[llvm-objcopy][NFC] More error cleanup
ClosedPublic

Authored by rupprecht on Feb 15 2019, 5:15 PM.

Details

Summary

This removes calls to error()/reportError() in the main driver (llvm-objcopy.cpp) as well as the associated argv-parsing (CopyConfig.cpp). logAllUnhandledErrors() is now the main way to print errors.

There are still a few uses from within the per-arch drivers, so we can't delete them yet... but almost!

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Feb 15 2019, 5:15 PM

I guess this is okay, but I'm wondering if it's gone too far? The majority of cases you've changed here are in the driver layer, so wouldn't likely ever be librarified. What's the motivation for this round?

llvm/tools/llvm-objcopy/CopyConfig.cpp
224 ↗(On Diff #187120)

You've added an extra '.' at the end of this error message. I think our standard is (usually) to not have them. (We might want to tidy up the other messages that do have them already).

242 ↗(On Diff #187120)

Ditto.

355 ↗(On Diff #187120)

New full stop?

370 ↗(On Diff #187120)

Ditto

398 ↗(On Diff #187120)

Ditto.

464 ↗(On Diff #187120)

Ditto.

470 ↗(On Diff #187120)

Ditto.

554 ↗(On Diff #187120)

Ditto.

604 ↗(On Diff #187120)

Ditto.

rupprecht updated this revision to Diff 187459.Feb 19 2019, 3:28 PM
rupprecht marked 10 inline comments as done.
  • Fix error message punctuation

I guess this is okay, but I'm wondering if it's gone too far? The majority of cases you've changed here are in the driver layer, so wouldn't likely ever be librarified. What's the motivation for this round?

Nothing in particular -- I just don't see the need for several custom error() methods in this tool (or maybe any tool, for that matter), and I think it'd be simpler if there was just one way to do error propagation.

There is still a weak link though -- the per-arch drivers (e.g. ELF/ELFObjcopy.cpp) rely on CopyConfig, and CopyConfig.cpp currently relies on llvm-objcopy.h, but just for error handling. It's not a headers dependency (it's not CopyConfig.h that depends on llvm-objcopy.h), so it's not so bad, but I think it's simple enough to do and it makes it more straightforward that CopyConfig can exist completely w/o llvm-objcopy.h (which we may have to do if we librarify this).

llvm/tools/llvm-objcopy/CopyConfig.cpp
224 ↗(On Diff #187120)

llvm-objcopy's error() automatically adds a "." when printing the error message, but printing w/ the default handler does not automatically add it, and we assert it ends with "." in some tests.

I don't have a preference, the added "." was to match that behavior. I've gone ahead and fixed all the tests that were checking for a trailing "." instead and reverted these bits. I can do other files in a separate change.

jhenderson accepted this revision.Feb 20 2019, 2:16 AM

LGTM.

llvm/tools/llvm-objcopy/CopyConfig.cpp
224 ↗(On Diff #187120)

I could have sworn I double-checked that! Either way, moving away from trailing full-stops is probably the right thing to do, as I think the majority of error messages from other tools do not use them (which I think is wrong, but we should be consistent). Perhaps @jakehehrlich has got something to say on this?

This revision is now accepted and ready to land.Feb 20 2019, 2:16 AM
This revision was automatically updated to reflect the committed changes.