[PATCH] D99751: Add the TableGen assert statement, step 3

[Jan Svoboda via Phabricator via llvm-commits]

jansvoboda11 added a comment.

Thanks for working on this, Paul!

I left a couple of questions/comments inline.

Also, it would be nice to go through the clang-format and clang-tidy suggestions and applying those that fit into the TableGen coding style.



================
Comment at: llvm/lib/TableGen/TGParser.cpp:368
+  if (E.Assertion) {
+    CheckAssert(std::get<0>(*E.Assertion), std::get<1>(*E.Assertion), 
+                std::get<2>(*E.Assertion));
----------------
Replacing `using AssertionTuple = std::tuple<SMLoc, Init *, Init *>` with a regular struct with named members would make this easier to read/understand. Is there any particular reason you chose `std::tuple` instead?


================
Comment at: llvm/lib/TableGen/TGParser.cpp:428
       Error = resolve(*E.Loop, Substs, Final, Dest);
+
+    } else if (E.Assertion) {
----------------
Nit: superfluous newline?


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3331
 ///  MultiClassObject ::= DefInst
-///  MultiClassObject ::= MultiClassInst
 ///  MultiClassObject ::= DefMInst
----------------
This patch doesn't seem to change the (supposedly) recursive nature of multiclasses. If the comment is stale, I think it would be clearer to move the change into a separate NFC commit explaining the situation.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3443
       case tgtok::Let:
+      case tgtok::Assert:
         if (ParseObject(CurMultiClass))
----------------
It looks like the branches used to be lexicographically ordered. If there's no specific benefit to having this be the last element, it might be better to keep it at the top. WDYT?


================
Comment at: llvm/lib/TableGen/TGParser.h:41
+  /// assertion tuple.
   struct RecordsEntry {
     std::unique_ptr<Record> Rec;
----------------
Shouldn't this be an `union {}` or `llvm::PointerUnion`, since it only holds one type at a time? If so, it would be nice to extract into a prep patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99751