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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.cpp:389
 
-  return Buf.commit();
+  // TODO: Implement direct writing to the output stream(without intermediate
+  // memory buffer Buf).
----------------



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2416
+
+  // TODO: Implement direct writing to the output stream(without intermediate
+  // memory buffer Buf).
----------------



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2542
+  if (!Buf)
+    return createStringError(llvm::errc::not_enough_memory,
+                             "failed to allocate memory buffer of " +
----------------
Elsewehere in this file, we don't qualify `llvm::errc` with the `llvm::`. I don't think you need it here either.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2544
+                             "failed to allocate memory buffer of " +
+                                 Twine::utohexstr(TotalSize) + " bytes.");
+
----------------
LLVM coding standards state that errors shouldn't end in '.' - please delete the trailing one here.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2555
 
-  return Buf.commit();
+  // TODO: Implement direct writing to the output stream(without intermediate
+  // memory buffer Buf).
----------------



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2588-2590
+    return createStringError(llvm::errc::not_enough_memory,
+                             "failed to allocate memory buffer of " +
+                                 Twine::utohexstr(TotalSize) + " bytes.");
----------------
As above. Remove the `llvm::` qualifier from `llvm::errc` and delete the trailing full stop from the message.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2644
+
+  // TODO: Implement direct writing to the output stream(without intermediate
+  // memory buffer Buf).
----------------



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2694
+  if (!EmptyBuffer)
+    return createStringError(errc::not_enough_memory, Twine(0));
+
----------------
Please write a more useful error message. This will result in a useless message of something like "error: foo.o: 0". This is the same issue as I already commented on in the mach-o area.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2709-2711
+    return createStringError(llvm::errc::not_enough_memory,
+                             "failed to allocate memory buffer of " +
+                                 Twine::utohexstr(TotalSize) + " bytes.");
----------------
As above. Delete `llvm::` and the trailing full stop.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:518-520
+    return createStringError(llvm::errc::not_enough_memory,
+                             "failed to allocate memory buffer of " +
+                                 Twine::utohexstr(TotalSize) + " bytes.");
----------------
As above - no `llvm::` or `.`


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:527
+
+  // TODO: Implement direct writing to the output stream(without intermediate
+  // memory buffer Buf).
----------------



================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:152
   for (const NewArchiveMember &Member : NewMembers) {
-    // Internally, FileBuffer will use the buffer created by
-    // FileOutputBuffer::create, for regular files (that is the case for
-    // deepWriteArchive) FileOutputBuffer::create will return OnDiskBuffer.
+    // For regular files(as is the case for deepWriteArchive),
+    // FileOutputBuffer::create will return OnDiskBuffer.
----------------



================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:62-64
+    return createStringError(llvm::errc::not_enough_memory,
+                             "failed to allocate memory buffer of " +
+                                 Twine::utohexstr(TotalSize) + " bytes.");
----------------



================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:79
+
+  // TODO: Implement direct writing to the output stream(without intermediate
+  // memory buffer Buf).
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91028