[PATCH] D122832: [TableGen] Add a backend to help with target enums

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:64
+void EnumEmitter::run(raw_ostream &OS) const {
+  assert(Records.getClass("Enum") && "Enum class definition missing");
+
----------------
Is this assert important? It looks like the code will just work even if there are no enums. If you're really concerned, it might be better to check !Enums.empty after the getAllDerivedDefinitions?


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:76
+  auto Name = EM->getValueAsString("Name");
+  assert(!Name.empty() && "Empty enum member name");
+  return Name;
----------------
Should these asserts be fatal errors since they are under the control of the input file?


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:106
+  // Keep track of enumerator values and names, as duplicates would cause C++
+  // miscompilations. These would be caught later by the C++ compiler, but it's
+  // better to flag this as early as possible.
----------------
Only duplicate names would be caught by the compiler right? It's not an error to have duplicate values.


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:150
+       << " " << Param << ") {\n";
+    OS.indent(2) << "switch (" << Param << ") {\n";
+    OS.indent(2) << "default:\n";
----------------
Looking at other uses of OS.ident, I don't think we typically use it for small constant indentations. The spaces are just included in the strings. That would make the formatting more obvious from the tablegen source.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122832/new/

https://reviews.llvm.org/D122832