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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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,
----------------
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?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:556
+static Expected<sys::fs::TempFile>
+writeArchiveInFD(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
+                 bool WriteSymtab, object::Archive::Kind Kind,
----------------
jhenderson wrote:
> MaskRay wrote:
> > FD might be a bit of misnomer (to me). I'd expect FD to refer to a file descriptor...
> Perhaps `writeTemporaryArchive` (since it's an archive in a temp file)?
The names need redoing again, since the logic has moved around a bit. I recommend this becomes `writeArchive` or `writeARchiveToStream` and the function that writes to a file becomes `writeArchiveToFile` or simply leave as `writeArchive` since the signatures are different.

Also, `ArcName` doesn't appear to be used any more?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84858