jhenderson added a comment. In D142660#4541406 <https://reviews.llvm.org/D142660#4541406>, @jhenderson wrote:
> In D142660#4538936 <https://reviews.llvm.org/D142660#4538936>, @DiggerLin > wrote: > >>> 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". >> >> if I do not think it is better than my current implement, If I implement as >> your suggestion, I need another variable to recoded the where the string >> come from(from OBJECT_MODE or -X option value). otherwise when the invalid >> value of string , how can I report it is an invalid OBJECT_MODE" or >> "invalid -X option value" > > I'm not convinced you need to distinguish the two. If a user hasn't specified > the -X option, then they'll know it's the OBJECT_MODE environment variable. > Even if they have both, it should be obvious to a quick glance of the full > error message what the wrong value is (because you'd include it in the > message) and you could then it won't take long to find which of the two is > wrong (even if they don't know that -X trumps OBJET_MODE). That being said, I > still prefer the parse OBJECT_MODE first, report an error, then later read > the -X option and check for an error there. It looks like you've ignored this point in your most recent changes. I agree that having a variable to say whether the BitMode variable comes from OBJECT_MODE or -X is not great, and I'd prefer the `HasAIXOption` style you had before over it. However, I'd still rather the invalid OBJECT_MODE value be read and rejected upfront before even parsing the -X option. ================ Comment at: llvm/include/llvm/Object/ArchiveWriter.h:44 +enum SymtabWritingMode { + NoSymtab, // Not write Symbol table. + NormalSymtab, // Write both 32-bit and 64-bit symbol table. ---------------- ================ Comment at: llvm/include/llvm/Object/ArchiveWriter.h:45 + NoSymtab, // Not write Symbol table. + NormalSymtab, // Write both 32-bit and 64-bit symbol table. + Bit32Only, // Only write the 32-bit symbol table. ---------------- This comment doesn't make sense for non-Big Archive archives, because regular archives only have one symbol table. There is no concept of a 32- or 64-bit one. Perhaps "Write the symbol table. For the Big Archive format, writes both 32-bit and 64-bit symbol tables." ================ Comment at: llvm/include/llvm/Object/ArchiveWriter.h:46-47 + NormalSymtab, // Write both 32-bit and 64-bit symbol table. + Bit32Only, // Only write the 32-bit symbol table. + Bit64Only // Only write the 64-bit symbol table. +}; ---------------- I'd prefix these two with `BigArchive`, or even just name them `BigArchive32` and `BigArchive64` respectively, to more clearly convey the fact that they are specific to that file format. ================ Comment at: llvm/lib/Object/ArchiveWriter.cpp:966 if (!isAIXBigArchive(Kind)) { - if (WriteSymtab) { + if (WriteSymtab != SymtabWritingMode::NoSymtab) { if (!HeadersSize) ---------------- Where this comparison is repeated more than once in the same function, it might be nice to store the value in a local boolean variable for use in all places instead of repeating the comparison. ================ Comment at: llvm/test/tools/llvm-ranlib/non-AIX-not-supportedwq-X-option.test:1 +## REQUIRES: !system-aix +## Test the -X option is not supported on non-AIX os. ---------------- Looks like there's a typo in your test name? ================ Comment at: llvm/test/tools/llvm-ranlib/non-AIX-not-supportedwq-X-option.test:6 + +# INVALID-X-OPTION: error: -X32 option not supported on non AIX OS ---------------- Nit: trailing whitespace 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
