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

Digger Lin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 11 10:14:32 PST 2022


DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/Archive.h:35-39
+namespace archive {
+const char Magic[] = "!<arch>\n";
+const char ThinMagic[] = "!<thin>\n";
+const char BigMagic[] = "<bigaf>\n";
+} // namespace archive
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > DiggerLin wrote:
> > > > jhenderson wrote:
> > > > > Rather than putting these in a new archive namespace (and then not putting the rest of the archive code in that namespace...), I'd suggest renaming the variables, and leaving them in the object namespace directly. Suggested names would be "ArchiveMagic", "ThinArchiveMagic", and "BigArchiveMagic" (optionally using "Ar" instead of "Archive", if you want something more succinct).
> > > > thanks
> > > No need to change this at this point, but noting that a later patch to rename Archive to UnixArchive or similar, should probably result in renaming `ArchiveMagic` to `UnixArchiveMagic`.
> > since Archive.h is  included in the files of llvm tools.  "Magic" is a common name. if the source code of llvm tools define global "Magic", it will be a conflict.  I think it is better to change  from "Magic" to "ArchiveMagic" now. , and change to UnixArchiveMagic in later patch.
> I think that's what I said to do? (i.e. as-is, this bit of the patch is fine, just remember to rename the Magic variable in a later patch)
thanks.


================
Comment at: llvm/include/llvm/Object/Archive.h:81
+template <typename T>
+class CommonMemHeaderFieldAPI : public AbstractArchiveMemberHeader {
+public:
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > DiggerLin wrote:
> > > > 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
> > > Fair point about the compilation issue. Maybe we should just move the `A` classes out of their parent classes? I'm not sure the nesting really gives us that much, and it seems to be making things more complex. What do you think?
> > yes, I think it seems making thing more complex.
> > 
> > what you concern about 
> > 
> > > 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.
> > 
> > a), I can change the 
> > 
> > ```
> > private:
> >   struct ArMemHdrType {
> >   }
> > ```
> > to public to avoid te friend. But I am prefer keeping the private for "struct ArMemHdrType"
> > 
> > b) I have added a helper function 
> >   const T &getDerivedMemberHeader() const  
> > to avoid "repeatedly do const T &DerivedMemberHeader = static_cast<const T &>(*this); in the get* methods ".
> > 
> > 
> > 
> > I'm not sure the nesting really gives us that much, and it seems to be making things more complex
> 
> I think you've misunderstood this. When I said it seems to be making things more complex, I mean that the current state of the code in this patch is making things more complex. By having the structs as private nested members, we are forced to use `friend` and have that helper method with the static_cast. This is more complex than not having those things. Making the classes independent would avoid the need for these bits, I believe, reducing complexity. The only cost is that other functions and classes have direct access to these classes, but I'm not sure how that's a real problem.
I think put the definition 
struct ArMemHdrType {
  }
in the struct
ArchiveMemberHeader {
}
is more clear to show that  the ArMemHdrType  is one part of ArchiveMemberHeader. 
and a small helper function is not a big problem.

anyway, I changed as your suggestion.



================
Comment at: llvm/test/Object/archive-big-extract.test:5
+# RUN: echo "content_of_evenlen" > evenlen_1
+# RUN: diff evenlen evenlen_1
----------------
jhenderson wrote:
> `cmp` tends to be more common than `diff` in tests I've seen doing similar things.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889



More information about the llvm-commits mailing list