[PATCH] D122746: [AIX][XCOFF] print unsupported message for llvm-ar big archive write operation

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/test/tools/llvm-ar/default-xcoff.test:1
+; REQUIRES: system-aix 
+;; Test llvm-ar do not big AIX archive write operation.
----------------
I'm confused why this is `system-aix` but the `XFAILed` tests are just `aix`. I supsect they all want to be `system-aix`?


================
Comment at: llvm/test/tools/llvm-ar/default-xcoff.test:2
+; REQUIRES: system-aix 
+;; Test llvm-ar do not big AIX archive write operation.
+
----------------
Let's use the term "Big" with upper-case 'B' to distinguish it from just a large archive.


================
Comment at: llvm/test/tools/llvm-ar/default-xcoff.test:11
+; CHECK: Unsupported big AIX write operation   
+
----------------
Nit: too many blank lines at EOF.


================
Comment at: llvm/test/tools/llvm-link/archivell.ll:1
-# RUN: llvm-ar cr %t.fg.a %S/Inputs/f.ll %S/Inputs/g.ll
+# RUN: llvm-ar --format=gnu cr %t.fg.a %S/Inputs/f.ll %S/Inputs/g.ll
 # RUN: not llvm-link %S/Inputs/h.ll %t.fg.a -o %t.linked.bc 2>&1 | FileCheck %s
----------------
What's the motivation for using `--format=gnu` in this and other test cases rather than the XFAIL option?


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:948
+    if (Kind == object::Archive::K_AIXBIG)
+      llvm_unreachable("Unsupported big AIX write operation");
     break;
----------------
`llvm_unreachable` is for use only when the program should never be able to reach this code. Clearly, here it can reach it (or you wouldn't need all those XFAILs in the tests). Instead, use `fail` like in the `case BSD:` case a few lines below.

Also, error messages should start with lower-case letter, as per the LLVM style guide.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1279
+          fail("exactly one archive should be specified");
+
+        ArchiveSpecified = true;
----------------
Undo this change. It's not relevant to the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122746