[PATCH] D75742: [Object] Support large archive members

[James Henderson via Phabricator via llvm-commits]

jhenderson marked an inline comment as done.
jhenderson added inline comments.


================
Comment at: llvm/unittests/Object/ArchiveTest.cpp:66
+  auto Child = (*A)->child_begin(ChildErr);
+  ASSERT_THAT_ERROR(std::move(ChildErr), Succeeded());
+
----------------
grimar wrote:
> Not strong suggestion:
> 
> ```
>   MemoryBufferRef Source(ArchiveWithMember, "regular");
>   Expected<std::unique_ptr<Archive>> A = Archive::create(Source);
>   ASSERT_THAT_EXPECTED(A, Succeeded());
>   Error ChildErr = Error::success();
>   auto Child = (*A)->child_begin(ChildErr);
>   ASSERT_THAT_ERROR(std::move(ChildErr), Succeeded());
> ```
> 
> This part is almost similar to one in `ArchiveChildGetSizeThinArchive` and `ArchiveChildGetSizeRegularArchive`.
> Should this code be moved to a helper that returns a `Child`?
I considered that. Unfortunately, ASSERT_* macros return void, so can't be inside other functions that return something else. Consequently, we have to put them in yet another function, then check to see if the thing is valid anyway (rendering the ASSERT pointless), and the logic becomes somewhat complex and hard to follow. I don't think there's much benefit as a result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75742