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

[Paul C. Anagnostopoulos via Phabricator via llvm-commits]

Paul-C-Anagnostopoulos added inline comments.


================
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));
----------------
jansvoboda11 wrote:
> 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?
No good reason to use a tuple. The next step is to switch to a struct. 


================
Comment at: llvm/lib/TableGen/TGParser.cpp:428
       Error = resolve(*E.Loop, Substs, Final, Dest);
+
+    } else if (E.Assertion) {
----------------
jansvoboda11 wrote:
> Nit: superfluous newline?
I like a line break between two arms of an 'if' when some arms are many lines.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3331
 ///  MultiClassObject ::= DefInst
-///  MultiClassObject ::= MultiClassInst
 ///  MultiClassObject ::= DefMInst
----------------
jansvoboda11 wrote:
> 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.
A separate revision just to remove an incorrect comment? Okay.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:3443
       case tgtok::Let:
+      case tgtok::Assert:
         if (ParseObject(CurMultiClass))
----------------
jansvoboda11 wrote:
> 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?
I will order them alphabetically.



================
Comment at: llvm/lib/TableGen/TGParser.h:41
+  /// assertion tuple.
   struct RecordsEntry {
     std::unique_ptr<Record> Rec;
----------------
jansvoboda11 wrote:
> 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.
I will investigate this when I convert the assertion tuple to a struct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99751