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

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

Few nits. LGTM syntax-wise. I'm not very familiar with backend, so will defer to someone who has better idea.



================
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).
----------------
What's expected to happen if we have names `foo` and `FOO`?



================
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";
----------------
Either there's one extra `'` or one is missing. There's no closing quote for `Field.Name`.


================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:660
+    Table->Name = TableRec->getName();
+    Table->PreprocessorGuard = TableRec->getName().upper();
+    Table->CppTypeName = TableRec->getValueAsString("CppTypeName");
----------------
If would be good to check and warn about name clashes if we have records that differ by case only.


================
Comment at: utils/TableGen/SearchableTableEmitter.cpp:695
+
+  for (auto IndexRec : Records.getAllDerivedDefinitions("SearchIndex")) {
+    Record *TableRec = IndexRec->getValueAsDef("Table");
----------------
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 &`.


Repository:
  rL LLVM

https://reviews.llvm.org/D48013