[PATCH] D111889: [AIX] Support of Big archive (read)

[Digger Lin via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/Archive.h:151
 
 class Archive : public Binary {
   virtual void anchor();
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Rather than be a potential concrete type, I think it would be cleaner if `Archive` were an abstract type, with concrete implementations for Big and traditional (better name required) archives. With Archive being a concrete type, there's a risk of slicing when someone thinks they've got a traditional archive, but actually have a BigArchive.
> > > 
> > > This then allows some somewhat better (in my opinion) locations for certain things.
> > if order to not modify a lot of tools code, we make Archive an abstract type. This patch is big enough. we can implement a derived class GNUArchive class derived from archive(as your suggestion in EGuesnet's patch)  too in another separate patch.
> > 
> > and move the code 
> > 
> > 
> > 
> > 
> > >  mutable std::vector<std::unique_ptr<MemoryBuffer>> ThinBuffers;
> > 
> > >   std::vector<std::unique_ptr<MemoryBuffer>> takeThinBuffers() {
> > >   return std::move(ThinBuffers);
> > > }
> > > 
> > > 
> > 
> > 
> > 
> > from class  Archive to class GNUArchive the other separate patch.
> If you don't want to do that change in this patch (which is fair enough), could you please do it in a separate precursor patch to this, please?
> 
> I'd probably not call the concrete class GNUArchive, since it will, at least for now, act as the concrete class used for a number of closely-related archive formats, not all of which are GNU-related. UNIXArchive might be more accurate. In the future, we might end up splitting that further into additional types, e.g. COFFArchive, GNUArchive, BSDArchive etc).
I will create another patch for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889