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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 2 inline comments as done.
sameerarora101 added inline comments.


================
Comment at: llvm/include/llvm/Object/ArchiveWriter.h:47
+                     bool Deterministic, bool Thin,
+                     std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
 }
----------------
MaskRay wrote:
> If `OldArchiveBuf` is not used (for now), delete it. It is a good idea adding a comment that this function is similar to writeArchive but has a different return type.
Removed `OldArchiveBuf` from signature, thanks.


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


================
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:
> 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?
Yup, removed `ArcName` from the signature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84858