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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I think we're basically there. Just some nits to sort out.



================
Comment at: llvm/include/llvm/Object/Archive.h:81
+public:
+  CommonArchiveMemberHeader(const Archive *Parent, const T* RawHeaderPtr)
+      : AbstractArchiveMemberHeader(Parent), ArMemHdr(RawHeaderPtr){};
----------------
Please remember to clang-format each diff.


================
Comment at: llvm/include/llvm/Object/Archive.h:105
+class ArchiveMemberHeader : public CommonArchiveMemberHeader<UnixArMemHdrType> {
+
+public:
----------------
Please delete blank lines at starts of classes/functions/blocks etc.


================
Comment at: llvm/lib/Object/Archive.cpp:208
+    return NameLenOrErr.takeError();
+
+  uint64_t NameLen = NameLenOrErr.get();
----------------
I'd delete this blank line, so that the setting of the actual name length is tied to the bit before.


================
Comment at: llvm/lib/Object/Archive.cpp:211
+
+  // If name length is odd, pad with '\0' to get an even length. After padding,
+  // there is the name terminator "`\n".
----------------



================
Comment at: llvm/lib/Object/Archive.cpp:213
+  // there is the name terminator "`\n".
+
+  uint64_t NameLenWithPadding = alignTo(NameLen, 2);
----------------
I'd probably delete this blank line.


================
Comment at: llvm/lib/Object/Archive.cpp:218
+      StringRef(ArMemHdr->Name, NameLenWithPadding + NameTerminator.size());
+
+  if (!NameStringWithNameTerminator.endswith(NameTerminator)) {
----------------
Ditto.

Deleting these two blank lines helps tie together the padding/terminator logic with the associated comment.


================
Comment at: llvm/lib/Object/Archive.cpp:353
+
+  return *SizeOrErr + alignTo(*NameLenOrErr,2);
+}
----------------
Still not clang-formatted...


================
Comment at: llvm/lib/Object/Archive.cpp:369-371
+  if (!AccessModeOrErr) {
+    return AccessModeOrErr.takeError();
   }
----------------
No braces for single line ifs.


================
Comment at: llvm/lib/Object/Archive.cpp:425
+  const char *NextLoc =
+      reinterpret_cast<const char *>(ArMemHdr) + alignTo(Size,2);
+
----------------
Still not clang-formatted...


================
Comment at: llvm/lib/Object/Archive.cpp:669-673
+  if (kind() != K_AIXBIG) {
+    return new ArchiveMemberHeader(this, RawHeaderPtr, Size, Err);
+  } else {
+    return new BigArchiveMemberHeader(this, RawHeaderPtr, Size, Err);
+  }
----------------



================
Comment at: llvm/lib/Object/Archive.cpp:715
   // contents.
+
   Format = K_GNU;
----------------
Delete this blank line.


================
Comment at: llvm/lib/Object/Archive.cpp:1158
+  StringRef RawOffset = getFieldRawString(ArFixLenHdr->FirstChildOffset);
+
+  if (RawOffset.getAsInteger(10, FirstChildOffset))
----------------
I'd delete this blank line, so that `RawOffset` is tied to the bit it's used in.


================
Comment at: llvm/lib/Object/Archive.cpp:1164
+  RawOffset = getFieldRawString(ArFixLenHdr->LastChildOffset);
+
+  if (RawOffset.getAsInteger(10, LastChildOffset))
----------------
Ditto.


================
Comment at: llvm/lib/Object/Archive.cpp:1182
+
+#undef getFieldRawString
----------------
This undef is now unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889