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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 3 inline comments as done.
frasercrmck 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");
+
----------------
craig.topper wrote:
> 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?
I don't think it's important, no. From what I can tell, `getAllDerivedDefinitions` reports a fatal error if the class isn't defined. That's better than an assert.


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


================
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.
----------------
craig.topper wrote:
> Only duplicate names would be caught by the compiler right? It's not an error to have duplicate values.
It's only a error when requesting a ToString method, due to the way we use a `switch` over the enum. So it's an "opt-in" error, in that sense. Maybe the comment isn't making that clear?


================
Comment at: llvm/utils/TableGen/EnumEmitter.cpp:150
+       << " " << Param << ") {\n";
+    OS.indent(2) << "switch (" << Param << ") {\n";
+    OS.indent(2) << "default:\n";
----------------
craig.topper wrote:
> 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.
Ah yeah, you're right. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122832