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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added a comment.

Thanks for the updates. I only have two nitpicks left now.



================
Comment at: llvm/lib/TableGen/Record.cpp:1301
+        PrintFatalError(CurRec->getLoc(),
+                        Twine("!getdagarg index is out of range 0...") +
+                            std::to_string(Dag->getNumArgs()) + ": " +
----------------
simon_tatham wrote:
> 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)"?
I see that you've done a simpler thing than I suggested, by just writing 0...(n-1) in place of 0...n.

But this leads to a bad error message when n=0. Remember that a dag node need not have any arguments at all:
```<stdin>:1:14: error: !getdagarg index is out of range 0...4294967295: 0
def foo; def test { dag x = (foo); int y = !getdagarg<int>(x, 0); }
             ^
```


================
Comment at: llvm/lib/TableGen/TGLexer.h:53
+  equal,
+  question,  // = ?
+  paste,     // #
----------------
hliao wrote:
> simon_tatham wrote:
> > 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.
> The pre-merge check fails now once clang-format generates different changes from my original change cause we add `XDagGetArg` and `XDagGetName`. I could revert that part of change or make such formatting NFC change in advance before this patch.
I don't mind whether you do it in this commit, or separately, or not at all. But if you do it, please convert line pairs like this
```  minus,
  plus, // - +
```
into a form where the comments make sense:
```  minus, // -
  plus, // +
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151602