[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:387-388
 
-  return Buf.commit();
+  // TODO: Implement direct writing to the output stream
+  // TODO: (without intermediate memory buffer Buf).
+  Out.write(Buf->getBufferStart(), Buf->getBufferSize());
----------------
You don't need TODO: on this second line. Also, your comment seems to have wrapped far earlier than the 80 character column limit, so you should reflow the comment. clang-format can reflow comments that are too long, as needed, although I don't think it does the inverse (i.e. reflow comments that are shorter than needed), so the easiest thing to do is just delete the new line and run clang-format on the comment.


================
Comment at: llvm/tools/llvm-objcopy/COFF/Writer.cpp:390
+  Out.write(Buf->getBufferStart(), Buf->getBufferSize());
+  Out.flush();
+  return Error::success();
----------------
Do we actually need this explicit flush? It seems to me like streams should flush before destruction, so there's no need to explicitly do it here, I'd think.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:831
                         : getOutputElfType(In);
-  ArrayRef<uint8_t> BuildIdBytes;
-
----------------
@MaskRay deleted the build ID directory code in another patch (see https://reviews.llvm.org/D96310). We shouldn't be seeing that deletion in this patch too. Please rebase.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2417
+  Out.write(Buf->getBufferStart(), Buf->getBufferSize());
+  Out.flush();
+  return Error::success();
----------------
Same as above. Do we need the flush?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2550-2553
+  // TODO: Implement direct writing to the output stream
+  // TODO: (without intermediate memory buffer Buf).
+  Out.write(Buf->getBufferStart(), Buf->getBufferSize());
+  Out.flush();
----------------
Same as above re. flush and TODO comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2639
+  Out.write(Buf->getBufferStart(), Buf->getBufferSize());
+  Out.flush();
+  return Error::success();
----------------
Same as above re. flush.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:466
+
+    SmallVector<char, 0> Buffer;
+    raw_svector_ostream MemStream(Buffer);
----------------
I'm not sure if it ever happened, but there was some discussion on the mailing lists about making `0` the default value if the size wasn't specified. Please could you check whether you need this second parameter still.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:486
+
+  Out.flush();
+  return Error::success();
----------------
Same comment as above re. flush.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:517
+  if (!Buf)
+    return createStringError(llvm::errc::not_enough_memory, Twine(totalSize()));
+  memset(Buf->getBufferStart(), 0, totalSize());
----------------
If I'm not mistaken, this and equivalent messages elsewhere will just result in something like the following being printed:

`error: 'foo.o': 42`

You need to give the full description of the message (i.e. something like "failed to allocate memory buffer of XX bytes").


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:524-527
+  // TODO: Implement direct writing to the output stream
+  // TODO: (without intermediate memory buffer Buf).
+  Out.write(Buf->getBufferStart(), Buf->getBufferSize());
+  Out.flush();
----------------
Same comment re. todo comments and flush.


================
Comment at: llvm/tools/llvm-objcopy/Util.h:19-20
+
+inline Error writeToFile(StringRef OutputFileName,
+                         std::function<Error(raw_ostream &)> Write) {
+  if (OutputFileName == "-")
----------------
Why is this function now in a new header file? Could it not be declared in one of the existing headers (like llvm-objcopy.h) and defined in the corresponding cpp?


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:95
+static Expected<ArrayRef<uint8_t>>
+findBuildID(const CopyConfig &Config, const object::ELFFile<ELFT> &In) {
+  auto PhdrsOrErr = In.program_headers();
----------------
dblaikie wrote:
> Might be easier to review if this code was moved around in a separate preparatory commit. So the code changes would be separate from the code moving.
+1 - this code move is unrelated. Please do it in a separate patch. Some of it might even be deletable after rebasing on D96310).


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:212-213
   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.
+    // FileOutputBuffer::create, for regular files(that is the case for
+    // deepWriteArchive) will return OnDiskBuffer.
     // OnDiskBuffer uses a temporary file and then renames it. So in reality
----------------
What's going on with this comment change? It looks like you've messed it up a bit.

I think what you want is "For regular files (as is the case for deepWriteArchive), FileOutputBuffer::create will return OnDiskBuffer."


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:302
 
-    MemBuffer MB(ChildNameOrErr.get());
-    if (Error E = executeObjcopyOnBinary(Config, *ChildOrErr->get(), MB))
+    SmallVector<char, 0> Buffer;
+    raw_svector_ostream MemStream(Buffer);
----------------
Same comment as above.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:407-410
+    if (Error E = writeToFile(
+            Config.OutputFilename, [&](raw_ostream &OutFile) -> Error {
+              return ProcessRaw(Config, *BufOrErr->get(), OutFile);
+            }))
----------------
I don't understand this change? This seems to be doing something quite different to what it was doing before.


================
Comment at: llvm/tools/llvm-objcopy/wasm/Writer.cpp:77-80
+  // TODO: Implement direct writing to the output stream
+  // TODO: (without intermediate memory buffer Buf).
+  Out.write(Buf->getBufferStart(), Buf->getBufferSize());
+  Out.flush();
----------------
Same comment as above re. flush and todo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91028