[PATCH] D48013: TableGen/SearchableTables: Support more generic enums and tables

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: include/llvm/TableGen/SearchableTable.td:15
+// 1. (Generic) Enums. By instantiating the GenericEnum class once, an enum with
+// the name of the def is generated. Is is guarded by the preprocessor define
+// GET_name_DECL, where name is the name of the def (in upper case).
----------------
javed.absar wrote:
> nitpick - "Is is guarded by..."
Fixed, thanks.


================
Comment at: include/llvm/TableGen/SearchableTable.td:22-26
+// Both a primary key and additional secondary keys / search indices can also
+// be defined, which result in the generation of lookup functions. Their
+// declarations and definitions are all guarded by GET_name_DECL and
+// GET_name_IMPL, respectively, where name is the name of the underlying table
+// (in upper case).
----------------
tra wrote:
> What's expected to happen if we have names `foo` and `FOO`?
> 
Nothing in particular. They will use the same include guard name, like the code that existed before.

Having an enum and a table guarded by the same guard is actually something that happens with the translation of the old SearchableTable, and it's why I've moved the corresponding `#undef`s to be at the end of the generated file.

So I'm not sure that is something that needs a warning either? I mean, everything should still work if you have two definitions that differ only by case, it's just that you then cannot selectively "GET" only one of them from C++.


================
Comment at: include/llvm/TableGen/SearchableTable.td:77
+  //
+  //   def SomeEnum : GenericEnum {
+  //     let FilterClass = "SomeEnum";
----------------
javed.absar wrote:
> Maybe change to 'MyEnum' to be consistent with rest of the example.
Done.


================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:166-167
       else
-        PrintFatalError(SMLoc(), "bitfield too large to search");
+        PrintFatalError(Twine("bitfield '") + Field.Name +
+                            "too large to search");
       return "uint" + utostr(NumBits) + "_t";
----------------
tra wrote:
> Either there's one extra `'` or one is missing. There's no closing quote for `Field.Name`.
Fixed, thanks.


================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:695
+
+  for (auto IndexRec : Records.getAllDerivedDefinitions("SearchIndex")) {
+    Record *TableRec = IndexRec->getValueAsDef("Table");
----------------
tra wrote:
> Nit: I'd use `auto *IndexRec` or even `Record *IndexRec`, otherwise it makes one wonder whether we are creating unnecessary copies here and why it's not `auto const &`.
Done.


Repository:
  rL LLVM

https://reviews.llvm.org/D48013