[PATCH] D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

This looks fine to me generally. Have a suggestion though:
in few places there are changes that are related to clang-formatting of original code it seems.
I'd commit them as a separate NFC cleanup (you don't need a review for that) and then rebase this diff.



================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:39
+  bool ErrorReported = false;
+  auto ErrHandler = [&](const Twine &) { ErrorReported = true; };
+
----------------
Perhaps just fail inside?


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:50
+  T *TypedFile = dyn_cast<T>(Obj.get());
+  ASSERT_TRUE(TypedFile);
+
----------------
You can avod having this `ASSERT_TRUE` if you use `cast<T>`, I think.
Also, you can use `T &` instead of a pointer, because it is expected that the value is always non-null.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:99
+   Type:     ET_REL
+   Machine:  EM_X86_64)",
+      elf::executeObjcopyOnBinary,
----------------
Do you need `Machine:  EM_X86_64`? By default it is `EM_NONE`, so it should work without an explicit value I guess.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88827