[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation
Duncan P. N. Exon Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 12 18:15:28 PDT 2020
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
More information about the cfe-commits
mailing list