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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added inline comments.


================
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
----------------
nhaehnle wrote:
> 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?
That's actually more like how I had it in the first version of the patch, and I had second thoughts and changed it //to// this :-) so I'm already on the fence and could easily be persuaded to change it back again.

My thought was that some use cases wouldn't care about the name at all (e.g. an entire backend might use dag-typed data for some purpose that never gives a name to any argument), and those users would find it more convenient if they could get the actual arguments by just saying `node['args'][n]` instead of having to say `node['args'][n][0]` (in your version) or `node['args'][n]['value']` (as I originally had it). So I moved the names out into a separate array that you'd only have to look at if you cared about names at all.

On the other hand, I can certainly see the counterargument – if you //do// care about names, it's nicer to have a single array to iterate over. I'm happy to change it to your style.


================
Comment at: lib/TableGen/JSONBackend.cpp:93
+      obj["kind"] = "varbit";
+      obj["variable"] = Var->getName();
+      obj["index"] = VarBit->getBitNum();
----------------
nhaehnle wrote:
> I think this should be "var" instead of "variable", for consistency.
Another thing I was trying to do (but forgot to actually mention anywhere) was to arrange for all the field names that depend on "kind" //not// to be the same as each other, as a means of error detection – it would stop a user accidentally mistaking a var for a varbit, by absentmindedly retrieving `node['var']` and forgetting there was another field to look at too. But that's quite a marginal consideration in the first place, and also I admit this particular choice of two different names was terrible :-) so yes, I'm happy to change over to being consistent.


================
Comment at: test/TableGen/JSON-check.py:49
+if fails != 0:
+    sys.exit("{} checks failed".format(fails))
+else:
----------------
nhaehnle wrote:
> Cool, I didn't know this was possible.
You mean passing a string to `sys.exit`? That was new to me as well quite recently. It's one of those functions where as a C programmer I automatically assumed I already knew how its API would work, so it took me years to find a reason to read its docs!


https://reviews.llvm.org/D46054