jhenderson added inline comments.
================
Comment at: llvm/test/tools/llvm-ranlib/aix-X-option.test:3
+## Test the -X option.
+## The option specifies the type of object file on which llvm-ranlib will
operate.
+
----------------
Nit: trailing whitespace
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1394
object::Archive::K_AIXBIG) {
Match = *(*ArgIt + 2) != '\0' ? *ArgIt + 2 : *(++ArgIt);
BitMode = getBitMode(Match);
----------------
Unrelated to this patch, but I spotted this when comparing the llvm-ar and
llvm-ranlib parsing logic: what happens if -X is the very final argument,
without a value? E.g. `llvm-ar (any other args...) -X`?
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1437
+ const char *Xarg = arg.data();
+ if (Xarg[0] == '\0')
+ BitMode = BitModeTy::Unknown;
----------------
If I'm reading this correctly, llvm-ranlib will accept "-X32" but reject "-X
32". Is that intentional? If so, what's the motivation behind it (I would say
that there needs to be a motivation other than "that's what AIX ranlib does)?
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1442
+
+ // -X option in ranlib do not accept "any"
+ if (BitMode == BitModeTy::Unknown || BitMode == BitModeTy::Any)
----------------
I know that AIX ranlib doesn't accept "any" whereas I believe AIX ar does. I
wonder though what the benefit is of preventing llvm-ranlib from accepting
"any"? Dropping that special case would simplify the code.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1463
+ if (object::Archive::getDefaultKindForHost() == object::Archive::K_AIXBIG) {
+ // If not specify -X option, get BitMode from enviorment variable
----------------
Is there a particular reason that this is after the command-line option parsing
for llvm-ranlib, but before it in llvm-ar? If this were before the option
parsing, you wouldn't need the `HasAixXOption` variable.
================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1469-1471
+ if (BitMode == BitModeTy::Any || BitMode == BitModeTy::Unknown)
+ fail("The OBJECT_MODE environment variable has an invalid setting. "
+ "OBJECT_MODE must be 32, 64, or 32_64.");
----------------
This is an inconsistency with llvm-ar's current behaviour: llvm-ar treats
`Unknown` as 32, whereas here you're outright rejecting it. I'm not convinced
this inconsistency makes sense, nor do I see it benefiting the user. In my
opinion the two should do the same thing. I think a garbage string should be
outright rejected in both cases. An empty string for the environment variable
or command-line option should probably be rejected too, but I'm okay with it
being accepted, if AIX ranlib behaves that way. However, I think llvm-ranlib
and llvm-ar should do the same whatever that is.
Once these inconsistencies have been ironed out, I think it'll be a lot simpler
to share code between the two tools.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142660/new/
https://reviews.llvm.org/D142660
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
