[PATCH] D84858: [llvm-libtool-darwin] Refactor ArchiveWriter

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:626-636
   // At this point, we no longer need whatever backing memory
   // was used to generate the NewMembers. On Windows, this buffer
   // could be a mapped view of the file we want to replace (if
   // we're updating an existing archive, say). In that case, the
   // rename would still succeed, but it would leave behind a
   // temporary file (actually the original file renamed) because
   // a file cannot be deleted while there's a handle open on it,
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > I don't think this needs doing - the `MemoryBuffer` will be destroyed once it goes out-of-scope (I'm assuming that `reset` effectively just clears the buffer). In that case, there's no need for it to even be passed in. In fact, looking up the stack, there's no need for it to be passed into any of these functions as far as I can see?
> Isn't it the case that we would need `OldArchiveBuf.reset();` in `writeArchive` and not in `writeARchiveToStream`? This is because, as per the comment, we need to close all handles to the archive before calling `Temp->keep(name)`, right? I have shifted this to `writeArchive` now. Please let me know if this is a wrong approach.
Makes sense. Apparently I blanked half the comment...!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84858