[PATCH] D83520: [llvm-libtool-darwin] Allow flattening archives

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:97-102
+    Error Err = Error::success();
+    for (const object::Archive::Child &Child : Lib.children(Err))
+      if (Error E = addChildMember(Members, Child))
+        return createFileError(FileName, std::move(E));
+    if (Err)
+      return createFileError(FileName, std::move(Err));
----------------
smeenai wrote:
> smeenai wrote:
> > Thinking through this a little more, `Err` is passed to the iterator to indicate iteration issues, so you should be checking it inside the loop in each iteration (probably before you call `addChildMember`). You still need to do an explicit check of `Err` afterward, so that in the case where the loop didn't iterate at all, the Error is still considered checked (it's guaranteed to be a success in that case, but you still need to do an explicit boolean check to mark it as checked). Perhaps the following would suffice for that? I'm not sure what pattern other parts of the codebase use for this.
> > 
> > ```
> > static_cast<bool>(Err);
> > ```
> Ah, never mind. The iteration will stop in case of an error, so having the error check after the loop is perfectly fine. (See llvm/include/llvm/ADT/fallible_iterator.)
I had basically this same thought originally too, but this is the pattern for looping through archive members elsewhere, so I realised it was safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83520