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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

>> While building this, there's a warning about an unhandled switch:
>
> It occurs on ArchiveWriter.cpp. I implement a way to write Big Archive. If you think it is better, I can add some code to remove warning, but my goal is to implement read and write for Big Archive, so it will be corrected in a future commit / PR.

Many people build with `-Werror`, so it would be good to handle the case even if it's explicitly ignored, like:

  case K_XCOFF:
    llvm_unreachable("Not handled yet");



>> Using a prebuilt binary should generally be avoided. You should create the archive at test run time, by using llvm-ar or some other tool (e.g. expanding yaml2obj to support this file format).
>
> I agree it should be avoided. Unfortunately, we cannot create archive at run time, as the write operation is not implemented yet (I work on it). Moreover, yaml2obj has not been ported on AIX (XCOFF object format). In future, when write operation will be implement for Big Archive, binary should be delete.

Generally I agree that prebuilt binaries should be avoided for testing (mostly due to being opaque), but there's also the case that we should have at least one prebuilt binary that we use to avoid symmetric bugs (imagine a bug in a bad archive being created being missed because the archive reader was also changed to read it incorrectly).



================
Comment at: llvm/include/llvm/Object/Archive.h:41
+  // clone() is used to create a new object identical to original.
+  virtual AbstractArchiveMemberHeader *clone() const = 0;
+  virtual ~AbstractArchiveMemberHeader(){};
----------------
The clone() method should return a std::unique_ptr<AbstractArchiveMemberHeader> to avoid raw memory management


================
Comment at: llvm/include/llvm/Object/Archive.h:231
+
+    bool operator==(const Child &other) const {
       assert(!Parent || !other.Parent || Parent == other.Parent);
----------------
There's a lot of misc formatting changes in this patch. I formatted everything in e41aaea26238d0a5cb19163863819786e24f0e02, so if you rebase past that, the diff will be cleaner.


================
Comment at: llvm/include/llvm/Object/Archive.h:391
+  // so we need to memorize position.
+  static uint64_t CurrentLocation;
+
----------------
Does this need to be static? e.g. this could break if any tool needed to read two separate big archives


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100651