[PATCH] D91028: [llvm-objcopy][NFC] replace class Buffer/MemBuffer/FileBuffer with streams.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

This also looks generally OK to me (after fixing style issues mentioned by James). Few more monor nits from me inline.



================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:100
+
+  for (const auto &Phdr : *PhdrsOrErr) {
+    if (Phdr.p_type != ELF::PT_NOTE)
----------------
I'd not use `auto` here. You can use an actual type instead.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:104
+    Error Err = Error::success();
+    for (auto Note : In.notes(Phdr, Err))
+      if (Note.getType() == ELF::NT_GNU_BUILD_ID &&
----------------
The same, probably.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:136
+  return createStringError(EC, FullMsg.c_str(), std::forward<Ts>(Args)...);
+}
+
----------------
Do you need this helper? I think generally the direction is to get rid of error codes.
I am not sure if this method is usefull? You can just craft a better error message using `createStringError` in place.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:177
+    Path.push_back('\0');
+    return makeStringError(EC, "cannot link '%s' to '%s'", ToLink.data(),
+                           Path.data());
----------------
Perhaps, `link`->`rename`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91028