[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:43
 
-class ArchiveMemberHeader {
+class AbstractArchiveMemberHeader {
+protected:
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Whilst looking at the later points, it occurred to me that we could solve some of the duplication, by doing something like the following:
> > > 
> > > # Change `AbstractArchiveMemberHeader` to take a template parameter, namely the underlying ArMemHdrType used in the subclasses.
> > > # Have the concrete classes pass in their private member type as the template parameter.
> > > # Create a new base class, that AbstractArchiveMemberHeader<T> inherits from.
> > > # Push the virtual interface into that class.
> > > # Move the functions that are duplicated in the two subclasses into the templated class. The subclasses should then only contain the sets of functions that actually have to be different, as opposed to just needing to be different due to neeeding to have slightly different template classes.
> > > 
> > > What do you think? Going with this approach, I'd suggest names like `ArMemberHeaderInterface`, and `AbstractArMemberHeader`, but other name ideas are welcome.
> > I think your method call "Curiously recurring template pattern" (abbr as CRTP),  I have considerate the method before.  but I give up when I tried to implement it. The reason as 
> > 1. Using CRTP, it will create two different base class AbstractArchiveMemberHeader after template instantiation .  there is no abstract class for both ArchiveMemberHeader and BigArchiveMemberHeader ,  if there is no common abstract class.
> > How do we deal with 
> > 
> > std::unique_ptr<AbstractArchiveMemberHeader> Header;
> > 
> > in 
> > 
> > ```
> >   class Child {
> >     friend Archive;
> >     friend AbstractArchiveMemberHeader;
> > 
> >     const Archive *Parent;
> >     std::unique_ptr<AbstractArchiveMemberHeader> Header;
> >     /// Includes header but not padding byte.
> >     StringRef
> > ```
> > using a template too ?
> It's similar to CRTP, but isn't CRTP. CRTP is when a class passes itself as a template parameter, e.g.
> ```
> class Derived : public Base<Derived> {};
> ```
> You've missed in my suggestion the addition of an additional base class. Here's a sketch of what I mean:
> ```
> // Base interface. Refer to this in client code.
> class ArchiveMemberHeaderInterface {
>   // pure virtual functions only
> };
> 
> // Templated helper class for common functionality
> template <typename FormatSpecificHeader>
> class AbstractArchiveMemberHeader : public ArchiveMemberHeaderInterface {
>   // concrete common functions
> };
> 
> class ArchiveMemberHeader : public AbstractArchiveMemberHeader<ArMemHdrType> {
>   // implementations of format-specific functionality
> };
> 
> class BigArchiveMemberHeader : public AbstractArchiveMemberHeader<BigArMemHdrType> {
> };
> ```
> See points 3 and 4 above. Nothing would ever directly reference `AbstractArchiveMemberHeader` directly, except in that inheritance.
I think I got your suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889