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

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

nhaehnle accepted this revision.
nhaehnle added a comment.
This revision is now accepted and ready to land.

Great, LGTM!



================
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
----------------
simon_tatham wrote:
> 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.
Yeah, I admit it was really a minor thing. Thanks for changing it though!


https://reviews.llvm.org/D46054