jhenderson added a comment. In D142660#4535693 <https://reviews.llvm.org/D142660#4535693>, @stephenpeckham wrote:
> I don't see any reason to check the OBJECT_MODE environment variable if the > -X flag is used. What would the error be: "You specified a valid -X flag, > but by the way, OBJECT_MODE is set to an invalid value"? The error would be "invalid value for OBJECT_MODE environment variable" (or something to that effect), which would mean the user fixes their environment, just the same as if they hadn't used -X. I just want to emphasise though that my main concern is that llvm-ar and llvm-ranlib are being inconsistent - one of them checks the environment variable first, the other checks the command-line option first, and this seems wrong - they should do the same thing. By reading and checking the environment variable first, it simplifies the code logic (no need for `HasAIXXOption` for example). As an alternative (but I think adds unnecessary complexity, due to needing an extra variable), you could have both tools read the environment variable into a string variable, then, if the -X option is present, overwrite that variable, and finally feed that string into the parsing code that converts into a `BitMode` value. If the string is invalid, the parsing code could report an error along the lines of "invalid OBJECT_MODE or -X option value". > I think all the commands that examine XCOFF files (llvm-ar, lllvm-ranlib, > llvm-readobj, llvm-objdump, llvm-nm, etc.) should recognize "32", "64", > "32_64", and "any". I don't think it's necessary to recognize "d64", even if > AIX commands do. In addition, I wouldn't bother recognizing an XCOFF file > with the magic number for a discontinued 64-bit object. That means that > "32_64" and "any" have the same behavior. If -X is specified and does not > have one of the 4 specified values, a usage message should be printed. If -X > is not specified but OBJECT_MODE is in the environment, a message should be > printed if the value is not one of the 4 specified values. Yes, to be clear, I'm not advocating that "d64" support should be added (I'm not opposed to it, should there be a use-case for it either). 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
