dexonsmith added inline comments.
================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3845 + IS_POSITIVE != DEFAULT_VALUE && this->KEYPATH != DEFAULT_VALUE) \ + Args.push_back(StringAllocator(Twine(PREFIX_TYPE[0]) + NAME)); +#include "clang/Driver/Options.inc" ---------------- Bigcheese wrote: > It's a little sad that we need to allocation every string just because of the > `-`. We definitely need to be able to allocate strings for options with data, > but it would be good if we could just have the strings with `-` prefixed in > the option table if that's reasonable to do. I want to highlight @Bigcheese's comment again so it doesn't get lost. I think a reasonable name would be `SPELLING`. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3903 + Args.push_back(StringAllocator(Twine(PREFIX_TYPE[0]) + NAME)); \ + Args.push_back(StringAllocator(DENORMALIZER(this->KEYPATH))); \ + } \ ---------------- The denormalizer should take the `StringAllocator` as an argument, and only allocate when necessary. ================ Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:327 + OS << "\n"; + OS << "#ifdef AUTO_NORMALIZING_OPTIONS"; + OS << "\n\n"; ---------------- Since this is meant to be included exactly once (as opposed to being an x-macro), perhaps this should be split out into a separate `.inc`. ================ Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:351-354 + OS << "#define " << MacroName << "(V, D) .Case(V, D)\n"; + emitValueTable(OS, OptionName, R.getValueAsString("Values"), + NormalizedValuesScope, NormalizedValues); + OS << "#undef " << MacroName << "\n"; ---------------- It seems unnecessary to generate macros here; you can just generate the code directly... But looking more closely, I wonder if we really need this generated code at all. Instead, can we create a table like this in Options.inc? ``` struct EnumValue { const char *Name; unsigned Value; }; struct EnumValueTable { const EnumValue *Table; unsigned Size; }; static const EnumValue RelocationModelTable[] = { {"static", static_cast<unsigned>(llvm::Reloc::Static)}, {"pic", static_cast<unsigned>(llvm::Reloc::_PIC)}, // ... }; static const EnumValue SomeOtherTable[] = { {"name", static_cast<unsigned>(clang::SomeOtherEnumValue)}, // ... }; static const EnumValueTable *EnumValueTables[] = { {&RelocationModelTable, sizeof(RelocationModelTable) / sizeof(EnumValue)}, {&SomeOtherTable, sizeof(SomeOtherTable) / sizeof(EnumValue)}, }; static const unsigned EnumValueTablesSize = sizeof(EnumValueTables) / sizeof(EnumValueTable); ``` We can then have this code directly in Options.cpp: ``` static llvm::Optional<unsigned> extractSimpleEnum( llvm::opt::OptSpecifier Opt, int TableIndex, const ArgList &ArgList, DiagnosticsEngine &Diags) { assert(TableIndex >= 0); assert(TableIndex < EnumValueTablesSize); const EnumValueTable &Table = *EnumValueTables[TableIndex]; auto Arg = Args.getLastArg(Opt); if (!Arg) return None; StringRef ArgValue = Arg->getValue(); for (int I = 0, E = Table.Size; I != E; ++I) if (ArgValue == Table.Table[I].Name) return Table.Table[I].Value; Diags.Report(diag::err_drv_invalid_value) << Arg->getAsString(ArgList) << ArgValue; return None; } ``` and change the normalizer contract to: - return an `Optional`, - take an `llvm::opt::OptSpecifier`, - take an index into a table (set to `-1` if there's no relevant table), and - be responsible for the boilerplate call to `getLastArg`. Simple enums would have `NORMALIZER` hardcoded to `extractSimpleEnum`. The handler code that calls it needs new arguments `TABLE_INDEX` and `TYPE` but also looks simpler this way: ``` if (auto MaybeValue = NORMALIZER(Opt##ID, TABLE_INDEX, Args, Diags)) \ this->KEYPATH = static_cast<TYPE>(*MaybeValue); \ else \ this->KEYPATH = DEFAULT_VALUE; ``` We could similarly have a single `serializeSimpleEnum` function in Options.cpp for a hardcoded `DENORMALIZER` for simple enums: ``` static const char *serializeSimpleEnum(int TableIndex, unsigned Value) { assert(TableIndex >= 0); assert(TableIndex < EnumValueTablesSize); const EnumValueTable &Table = *EnumValueTables[TableIndex]; for (int I = 0, E = Table.Size; I != E; ++I) if (Value == Table.Table[I].Value) return Table.Table[I].Name; llvm::report_fatal_error("good error message"); } ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79796/new/ https://reviews.llvm.org/D79796 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits