[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

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