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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 2 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/Archive.h:43
 
-class ArchiveMemberHeader {
+class AbstractArchiveMemberHeader {
+protected:
----------------
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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889