[PATCH] D123142: [llvm-ar] Fix thin archive being wrongly converted to a full archive

Mon Apr 18 23:51:38 PDT 2022

jhenderson added a comment.

In D123142#3451306 <https://reviews.llvm.org/D123142#3451306>, @gbreynoo wrote:

> Good idea @MaskRay, I've split that into https://reviews.llvm.org/D123778 and will update this one when D123778 <https://reviews.llvm.org/D123778> is complete.

There is no particular reason to wait - you could rebase this patch on top of that one, so that this patch doesn't include the other changes that will have already landed. To indicate that this patch isn't intended as a standalone patch, you then use the "Edit Related Revisions" UI option to the right of hte patch description.

Holding off approving until that's happen, as it would be easier for me to review exactly what this patch is finally doing.

================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1023-1024
     std::unique_ptr<object::Archive> Archive = std::move(ArchiveOrError.get());
-    if (Archive->isThin())
+    if (Thin && !Archive->isThin())
+      fail("cannot convert a regular archive to a thin one");
+
----------------
gbreynoo wrote:
> jhenderson wrote:
> > I've not looked at the full code paths, but at an educated guess, moving this code here will cause this error to occur in other cases which don't add new members, such as `t` operations? Is that desirable (I'm inclined to say so, but at least want to flag up the possibility of an issue)?
> Good catch, this has been changed to only check on write commands.
Is there a test case you could add that would have failed with the previous version of this patch?

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

https://reviews.llvm.org/D123142