[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I glanced at the patch. The code seems reasonable.



================
Comment at: clang/test/lit.cfg.py:383
 
-# The llvm-nm tool supports an environment variable "OBJECT_MODE" on AIX OS, which
+# Some tool support an environment variable "OBJECT_MODE" on AIX OS, which
 # controls the kind of objects they will support. If there is no "OBJECT_MODE"
----------------



================
Comment at: llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp:488
+          Thin ? object::Archive::K_GNU : object::Archive::K_COFF,
+          /*Deterministic*/ true, Thin, nullptr, COFF::isArm64EC(LibMachine))) {
     handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
----------------



================
Comment at: llvm/test/tools/llvm-ranlib/non-AIX-not-supported-X-option.test:1
+## REQUIRES: !system-aix
+## Test the -X option is not supported on non-AIX os.
----------------
`UNSUPPORTED: system-aix`

I think the convention is to name `aix-X-option.test` and `non-AIX-not-supported-X-option.test` with a shared prefix to make identify such tests easier.

Perhaps the file can be renamed to `AIX-X-option-non-AIX.test` or similar.


================
Comment at: llvm/test/tools/llvm-ranlib/non-AIX-not-supported-X-option.test:4
+
+# RUN: not llvm-ranlib -X32 2>&1 | FileCheck --implicit-check-not="error:"  --check-prefixes=INVALID-X-OPTION %s
+
----------------
With one prefix, we prefer `--check-prefix=`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660