[PATCH] D46054: [TableGen] Add a general-purpose JSON backend.

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

nhaehnle added a comment.

Thanks, this already looks very good. I do have some suggestions though.



================
Comment at: docs/TableGen/BackEnds.rst:516-518
+  parenthesis of the dag initializer; ``args`` is an array of the
+  following arguments; and ``argnames`` is a list giving the
+  colon-suffixed name of each argument, if any. For example, in the
----------------
This is a minor point, but I suspect it would be slightly more convenient for consumers if this were instead represented as `[[arg, name], [arg, name], ...]`. So the example below would have `args` be `[[22, null], ["hello", "foo"]]`. What do you think?


================
Comment at: lib/TableGen/JSONBackend.cpp:46-49
+  /*
+   * Init subclasses that we return as JSON primitive values of one
+   * kind or another.
+   */
----------------
C-style comments are rather uncommon in LLVM; best to be consistent and use C++-style comments here (and below).


================
Comment at: lib/TableGen/JSONBackend.cpp:93
+      obj["kind"] = "varbit";
+      obj["variable"] = Var->getName();
+      obj["index"] = VarBit->getBitNum();
----------------
I think this should be "var" instead of "variable", for consistency.


================
Comment at: lib/TableGen/JSONBackend.cpp:172-179
+  for (const auto &D : Records.getDefs()) {
+    auto &Name = D.second->getNameInitAsString();
+    auto &Def = *D.second;
+    for (const auto &SuperPair : Def.getSuperClasses()) {
+      auto SuperName = SuperPair.first->getNameInitAsString();
+      instance_lists[SuperName].push_back(Name);
+    }
----------------
I think it would be slightly nicer to merge this into the loop above, to avoid iterating over the same data twice.


================
Comment at: test/TableGen/JSON-check.py:49
+if fails != 0:
+    sys.exit("{} checks failed".format(fails))
+else:
----------------
Cool, I didn't know this was possible.


https://reviews.llvm.org/D46054