dang marked 2 inline comments as done. dang 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" ---------------- dexonsmith wrote: > 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`. Yes I haven't forgotten just have not gotten around to it yet. I just want to do some form of suffix merging with the string the the OptTable receives ================ 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"; ---------------- dexonsmith wrote: > 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"); > } > ``` I originally wanted to generate the normalizers without the table to allow and arbitrary code fragment to be used as the normalized value (one that is potentially not a constant evaluated expression) and just regenerating the macros was the path of least resistance at the time. Having had a closer look at the existing cases of enum based options there doesn't seem to be a need for this functionality so I went ahead and did this. Although I haven't split the backends yet. 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