[PATCH] D58316: [llvm-objcopy][NFC] More error cleanup

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

In D58316#1401185 <https://reviews.llvm.org/D58316#1401185>, @jhenderson wrote:

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



================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:224
+    return createStringError(errc::invalid_argument,
+                             "Invalid architecture: '%s'.", Arch.str().c_str());
   return Iter->getValue();
----------------
jhenderson wrote:
> 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).
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58316