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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/Archive.h:81
+template <typename T>
+class CommonMemHeaderFieldAPI : public AbstractArchiveMemberHeader {
+public:
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Thanks for the class restructuring. I hope you agree that it looks better not having the duplicated code.
> > 
> > I think the class name needs changing. Apart from anything else, the name says nothing about archives. I also don't think "FieldAPI" makes a huge amount of sense to me (I see what you're trying to do though). I have two possible alternatives:
> > # My preferred approach would be to rename `ArchiveMemberHeader` to `UnixArchiveMemberHeader`, and then use `ArchiveMemberHeader` for this class's name.
> > # If renaming the `ArchiveMemberHeader` touches too much code, I'd suggest renaming this class to `CommonArchiveMemberHeader`.
> > 
> > Abbreviating "Archive" to "Ar" and "Member" to "Mem" is okay, in either case.
> > 
> > Additionally, you haven't got it quite right: the `T` parameter here should be the current `T::ArMemHdrType` instead, with the `ArMemHdr` being passed into this class's constructor (and potentially the instance in the derived clas isn't needed). That'll avoid the need for a) the `friend` declarations (I believe), and b) the need to repeatedly do `const T &DerivedMemberHeader = static_cast<const T &>(*this);` in the get* methods.
> if change  ArchiveMemberHeader -> UnixArchiveMemberHeader 
> I think we need to change Archive to UnixAchive too at this moment,  changing Archive to UnixAchive will cause a lot of llvm-* file changing.  (I think we need another separate  patch for changing  Archive to UnixAchive later).
> I am prefer method 2 now.
> 
> 
> 
> > Additionally, you haven't got it quite right: the T parameter here should be the current T::ArMemHdrType instead, with the ArMemHdr being passed into this class's constructor (and potentially the instance in the derived clas isn't needed). That'll avoid the need for a) the friend declarations (I believe), and b) the need to repeatedly do const T &DerivedMemberHeader = static_cast<const T &>(*this); in the get* methods.
> 
> 
> I do not think I can do as your suggestion,  for example : the 
> ArchiveMemberHeader are not instantiated complete,  ArchiveMemberHeader::ArMemHdrType will be incomplete type.
> 
> you can try to compile the code 
> 
> 
> ```
> class AbstractArchiveMemberHeader {
> };
> 
> template <typename T>
> class CommonArchiveMemberHeader : public AbstractArchiveMemberHeader {
>  T *a;
> }
> 
> class ArchiveMemberHeader
>     : public CommonArchiveMemberHeader<ArchiveMemberHeader::A> {
> 
>  struct A {
>   };
> }
> 
> class BigArchiveMemberHeader
>     : public CommonArchiveMemberHeader<BigArchiveMemberHeader::A> {
>    struct A {
>   };
> }
> ```
> 
> you will get error when compile
> 
> 
> 
please add ; after the definition of class of above code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889