[PATCH] D78983: [ELF] Add --print-archive-stats=

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: lld/ELF/InputFiles.cpp:1171
+  Error err = Error::success();
+  for (const Archive::Child &c : file->children(err)) {
+    (void)c;
----------------
MaskRay wrote:
> grimar wrote:
> > MaskRay wrote:
> > > alexshap wrote:
> > > >  1.  what about 
> > > >       std::distance(file->child_begin(err), file->child_end());  ?
> > > >  2. just curious why is the error consumed ? e.g. alternatively this could return Expected<...>
> > > 1. `difference_type` for these iterator types is not defined so `std::distance` can't be used:
> > > ```
> > > /usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_iterator_base_funcs.h:138:5: note: candidate template ignored: substitution failure
> > >  [with _InputIterator = llvm::fallible_iterator<llvm::object::Archive::ChildFallibleIterator>]: no type named 'difference_type' in 'std::iterator_trai
> > > ts<llvm::fallible_iterator<llvm::object::Archive::ChildFallibleIterator> >'
> > > ```
> > > 
> > > > 2. just curious why is the error consumed ? e.g. alternatively this could return Expected<...>
> > > 
> > > --whole-archive and a special case call `getArchiveMembers`. When the control flow reaches here, `err` will guaranteed to be success. For the rest the archive may be corrupted but we just don't care about the error here.
> > What about this?
> > 
> > ```
> > size_t ArchiveFile::getMemberCount() const {
> >   Error err = Error::success();
> >   auto range = file->children(err);
> >   // A comment about why `consumeError` is OK.
> >   consumeError(std::move(err));
> > 
> >   size_t count = 0;
> >   for (auto i = range.begin(), e = range.end(); i != e; ++i)
> >     ++count;
> >   return count;
> > }
> > ```
> `llvm::object::Archive::child_iterator` keeps a reference to the `Error` object. `consumeError(std::move(err))` has to be called after the iteration is done.
> 
> Using `for (auto i = range.begin(), e = range.end(); i != e; ++i)` can avoid an unused loop variable but it seems to become more complex. In the end I still prefer a range-based for loop, even if there is an unused loop variable.
> but it seems to become more complex.
It shows the intention better though (IMO).


================
Comment at: lld/ELF/InputFiles.cpp:1178
+  // This function is used by --print-archive-stats=, where an error does not
+  // really matter.
+  consumeError(std::move(err));
----------------
grimar wrote:
> What do you think if instead of adding a `size_t getMemberCount() const;` to `ArchiveFile` we add:
> 
> ```
> Archive* getArchiveFile() const;
> ```
> 
> And then just inline the logic of `getMemberCount()` to `writeArchiveStats()`?
> 
> (The logic is not used anywhere else and I can't say that such loops look nice,
> perhaps hiding it away from the general code might look better).
Thinking about it again, I am not sure it will look much better as we already have
`size_t getFetchedMemberCount() const { return seen.size(); }` anyways.


================
Comment at: lld/ELF/InputFiles.h:330
+  size_t getMemberCount() const;
+  size_t getFetchedMemberCount() const { return seen.size(); }
+
----------------
Member**s**?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78983