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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/redefine-symbol.test:93
+#MISSING-SYM-NAME: error: {{.*}}.rename.txt:2: missing new symbol name
+#NO-FILE: error: '{{.*}}.rename-none.txt': No such file or directory.
+
----------------
This will fail on Windows because "No such file or directory." is Linux specific. You can make it platform independent by replacing it with `{{[Nn]}}o such file or directy` (note the lack of full-stop and the capitalization).


================
Comment at: test/tools/llvm-objcopy/ELF/redefine-symbol.test:101
+#MULTIPLE-FILES-NEXT:   Value: 0x1008
\ No newline at end of file

----------------
Nit: file is missing new line at end.


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


================
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);
+}
----------------
There's already a `createFileError` in LLVM. Can you use that?


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

https://reviews.llvm.org/D57738