[PATCH] D57738: [llvm-objcopy] Add --redefine-syms

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with the noted fix.



================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:278-281
+      return make_error<StringError>(Filename + ":" +
+                                         std::to_string(LineNo + 1) +
+                                         ": missing new symbol name",
+                                     errc::illegal_byte_sequence);
----------------
jhenderson wrote:
> Use `createStringError` here. That also allows you to use printf-style formatting if you want.
> 
> Also, this error isn't an "illegal_byte_sequence" which is to do with character encoding. It should be something like "invalid_argument" or "parse_failed" or similar.
%lu is the wrong format for a size_t. It should be %zu.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:55-58
+Error createError(StringRef File, std::error_code EC) {
+  assert(EC);
+  return make_error<StringError>("'" + File + "': " + EC.message() + ".", EC);
+}
----------------
jhenderson wrote:
> There's already a `createFileError` in LLVM. Can you use that?
It doesn't need to be part of this change, but I wonder if it would make sense for a new `createFileError` overload to be added alongside the existing one that takes a `std::error_code`?


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

https://reviews.llvm.org/D57738