[PATCH] D151602: [TableGen] Add !getdagarg and !getdagname

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added a comment.

It's not quite clear whether you mean this patch to be an alternative to D151457 <https://reviews.llvm.org/D151457>, or to go together with it. The two changes would clearly conflict, but one of your comments in the other change (https://reviews.llvm.org/D151457#4374848) made it sound as if you meant to do both?

If they're alternatives, then I prefer this one, because I love the extra feature of looking up a dag argument by its name instead of its index. I can see that being really useful.



================
Comment at: llvm/lib/TableGen/Record.cpp:1301
+        PrintFatalError(CurRec->getLoc(),
+                        Twine("!getdagarg index is out of range 0...") +
+                            std::to_string(Dag->getNumArgs()) + ": " +
----------------
This error message schema leads to the slightly weird error seen in your test:
```error: !getdagarg index is out of range 0...3: 3```
I'd normally interpret the notation "0...3" as being inclusive at both ends, so it's confusing to be told that 3 is not in the range 0...3.

To avoid this wording difficulty, perhaps it's better to break up the error so that a negative index gets its own message (it's illegal no matter how many arguments the dag has), and a positive one gets a message like "!getdagarg index 3 out of range (dag has 3 arguments)"?


================
Comment at: llvm/lib/TableGen/TGLexer.h:53
+  equal,
+  question,  // = ?
+  paste,     // #
----------------
It isn't clear why this section had to be reformatted at all in this patch, but if you //are// going to put each enum entry on its own line, you could at least break up the comments in the same way so that each one has a comment with its own symbol.


================
Comment at: llvm/test/TableGen/getsetop.td:93
+  int notFound = !getdagarg<int>(in1, "x");
+#endif
 }
----------------
Needs some more tests:
* total mismatch of types, e.g. `!getdagarg<int>` applied to an argument that's a string, or `!getdagarg<Foo>` applied to a record type that's not an instance of class Foo at all.
* demonstrate retrieving a type by its superclass but not subclass


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151602