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

[Digger Lin via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Object/Archive.cpp:51
+void createMemberHeaderParseError(
+    const AbstractArchiveMemberHeader *ArcMemHeader, const char *RawHeaderPtr,
+    uint64_t Size, Error *Err) {
----------------
jhenderson wrote:
> "Ar" is a more common abbreviation for "Archive" than "Arc"
thanks


================
Comment at: llvm/lib/Object/Archive.cpp:292
 
-Expected<sys::TimePoint<std::chrono::seconds>>
-ArchiveMemberHeader::getLastModified() const {
-  unsigned Seconds;
-  if (StringRef(ArMemHdr->LastModified, sizeof(ArMemHdr->LastModified))
-          .rtrim(' ')
-          .getAsInteger(10, Seconds)) {
-    std::string Buf;
-    raw_string_ostream OS(Buf);
-    OS.write_escaped(
-        StringRef(ArMemHdr->LastModified, sizeof(ArMemHdr->LastModified))
-            .rtrim(" "));
-    OS.flush();
-    uint64_t Offset =
-        reinterpret_cast<const char *>(ArMemHdr) - Parent->getData().data();
-    return malformedError("characters in LastModified field in archive header "
-                          "are not all decimal numbers: '" +
-                          Buf + "' for the archive member header at offset " +
-                          Twine(Offset));
+#define getFieldRawString(T) StringRef(T, sizeof(T)).rtrim(" ")
+
----------------
jhenderson wrote:
> Use a (potentially templated) function, not a macro. This macro does nothing beneficial that a function can't do.
for the T is not type , it is concrete field of the ArMemHdrType , it can not be templated function.


================
Comment at: llvm/lib/Object/Archive.cpp:56
+    uint64_t Size, Error *Err) {
+  ErrorAsOutParameter ErrAsOutParam(Err);
+  if (Err) {
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Is this safe and correct given that this has been done at the start of the calling code too?
> > I can not get the comment , I am appreciate that if you can you explain more detail on it.
> I'm not too familiar with how `ErrorAsOutParameter` works, but the calling site for this function also has an `ErrorAsOutParameter`. As a result, the `Err` has been put inside more than one of these objects, which seems suspicious to me. It may not be safe (e.g. it might assert).
> 
> That being said, why not just have this function return an Error always, and go back to checking whether the function returned an `Error::success()` at the call sites? The name of this function implies an `Error` is created always.
having this function return an Error always is good idea , thanks. 


================
Comment at: llvm/lib/Object/Archive.cpp:342-352
+uint64_t ArchiveMemberHeader::getOffset() const {
+  uint64_t Offset =
+      reinterpret_cast<const char *>(ArMemHdr) - Parent->getData().data();
+  return Offset;
+}
+
+uint64_t BigArchiveMemberHeader::getOffset() const {
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > These two functions are identical. Could they be pushed into the base class? You could pass in the pointer to get the offset from.
> > it can not pushed into the base class and pass in the pointer to get offset from.
> > 
> > When you call from the Child  , for example
> > Header->getOffset() 
> > Header is AbstractArchiveMemberHeader, it do not have ArMemHdr
> Apologies, I think you misunderstood me, possibly I wasn't clear enoguh.
> 
> How about the following concrete points:
> 
> 1) Add a virtual function to `AbstractArchiveMemberHeader` called `getData()` or similar.
> 2) Implement this concrete function in the two subclasses, to return `reinterpret_cast<const char *>(ArMemHdr)`.
> 3) Don't make `getOffset` a virtual function, and instead implement it solely in the base class as follows:
> ```
> uint64_t AbstractArchiveMemberHeader::getOffset() const {
>   return getData() - Parent->getData().data();
> }
> ```
using the The curiously recurring template pattern (CRTP) .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111889