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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've stopped reviewing partway through this. There are a number of my previous comments that you've marked as done but haven't been addressed. Can you confirm that you've updated this to the latest diff you'd like me to review, please, before I proceed further?



================
Comment at: llvm/include/llvm/Object/Archive.h:107
+
+// Define file member header of aix big archive.
+class BigArchiveMemberHeader : public AbstractArchiveMemberHeader {
----------------
If I'm not mistaken, "aix" is usually written as "AIX", so we should do the same here (I may easily be wrong though).


================
Comment at: llvm/include/llvm/Object/Archive.h:117-126
+  Expected<StringRef> getRawName() const override;
+  Expected<StringRef> getName(uint64_t Size) const override;
+  Expected<uint64_t> getSize() const override;
+  Expected<uint64_t> getRawNameSize() const;
+
+  uint64_t getOffset() const override;
+  StringRef getRawAccessMode() const override;
----------------
I'd group these accessor functions as `getRaw...` in one block, and then `get...` (without raw) in another block, with a blank line separating teh two. I'd also suggest ordering within the blocks to match each other, as close as practical.


================
Comment at: llvm/include/llvm/Object/Archive.h:396
+    char LastChildOffset[20];  ///< Offset to last archive member.
+    char FreeOffset[20];     ///< Offset to first mem on free list.
+  };
----------------
Please address all clang-format problems in your modified/new code.


================
Comment at: llvm/include/llvm/Object/Archive.h:63
+  // Returns the size in bytes of the format-defined header of derived class.
+  virtual uint64_t getSizeOf() const { return 0; }
+
----------------
jhenderson wrote:
> It seems to me like this should be a pure virtual function, rather than returning 0 here - another archive kind in the future should explicitly say what the size of its member headers are (even if it is zero). By providing a default implementation, there's a real risk some future implementation won't override this, and will then have problems.
This has been marked as done, but isn't addressed in the latest diff. Please don't mark things as done until they've been addressed in an uploaded diff (if you select the Mark as Done checkbox, in the UI, and then upload the diff, the checked boxes will be submitted automatically when you upload the diff).


================
Comment at: llvm/include/llvm/Object/Archive.h:65
+
+  Archive const *Parent;
+  AbstractArchiveMemberHeader(const Archive *Parent) : Parent(Parent){};
----------------
jhenderson wrote:
> Please move data to one place, rather than sandwiched between class methods.
> 
> Also, does this need to be public? It wasn't before...
> Also, does this need to be public? It wasn't before...

This bit hasn't been addressed.


================
Comment at: llvm/include/llvm/Object/Archive.h:125
+  Expected<uint64_t> getNextOffset() const;
+  static bool classof(AbstractArchiveMemberHeader const *Header);
+
----------------
jhenderson wrote:
> Separate unrelated functions with blank lines.
Marked as Done but not addressed.


================
Comment at: llvm/lib/Object/Archive.cpp:53
 
+void GenerateMemberHeaderParseError(
+    const AbstractArchiveMemberHeader *ArcMemHeader, const char *RawHeaderPtr,
----------------
1) Lower-camel-case names for functions.
2) "create" is a more common term than "generate" for these sort of functions.


================
Comment at: llvm/lib/Object/Archive.cpp:56
+    uint64_t Size, Error *Err) {
+  ErrorAsOutParameter ErrAsOutParam(Err);
+  if (Err) {
----------------
Is this safe and correct given that this has been done at the start of the calling code too?


================
Comment at: llvm/lib/Object/Archive.cpp:58-59
+  if (Err) {
+    std::string Msg("remaining size of archive too small for next archive "
+                    "member header ");
+    Expected<StringRef> NameOrErr = ArcMemHeader->getName(Size);
----------------
Make this a StringRef (or event a Twine I believe would work), rather than `std::string`.


================
Comment at: llvm/lib/Object/Archive.cpp:61-66
+    if (!NameOrErr) {
+      consumeError(NameOrErr.takeError());
+      uint64_t Offset = RawHeaderPtr - ArcMemHeader->Parent->getData().data();
+      *Err = malformedError(Msg + "at offset " + Twine(Offset));
+    } else
+      *Err = malformedError(Msg + "for " + *NameOrErr);
----------------
I believe if you flip these around, you can do the suggested inline edit. Also note I added the braces for the old else (now the new if part), as I believe the consensus is that if you use braces for an if, you should for all its corresponding else parts too (and vice versa).


================
Comment at: llvm/lib/Object/Archive.cpp:82
   if (Size < sizeof(ArMemHdrType)) {
-    if (Err) {
-      std::string Msg("remaining size of archive too small for next archive "
-                      "member header ");
-      Expected<StringRef> NameOrErr = getName(Size);
-      if (!NameOrErr) {
-        consumeError(NameOrErr.takeError());
-        uint64_t Offset = RawHeaderPtr - Parent->getData().data();
-        *Err = malformedError(Msg + "at offset " + Twine(Offset));
-      } else
-        *Err = malformedError(Msg + "for " + NameOrErr.get());
-    }
+    if (Err)
+      GenerateMemberHeaderParseError(this, RawHeaderPtr, Size, Err);
----------------
I don't believe you need this `if` anymore, right?


================
Comment at: llvm/lib/Object/Archive.cpp:119
+  if (Size < sizeof(ArMemHdrType)) {
+    if (Err)
+      GenerateMemberHeaderParseError(this, RawHeaderPtr, Size, Err);
----------------
I don't believe you need this `if` anymore?


================
Comment at: llvm/lib/Object/Archive.cpp:111
+
+  if (Size < sizeof(ArMemHdrType)) {
+    if (Err) {
----------------
DiggerLin wrote:
> jhenderson wrote:
> > If I'm reading this rightly, this entire block could be moved into common code with the regular archive vareity. If it's not possible to put it into the base class constructor, put it in a helper function or method instead. 
>  it can not put into the base class constructor , for the definition of ArMemHdrType is different in the BigArchiveMemberHeader and ArchiveMemberHeader.
Isn't that what the `getSizeOf` function is for?


================
Comment at: llvm/lib/Object/Archive.cpp:125
+  }
+
+}
----------------
jhenderson wrote:
> Don't have trailing blank lines at ends of functions.
Marked as done, but not addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889