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

[Jan Svoboda via Phabricator via llvm-commits]

jansvoboda11 requested changes to this revision.
jansvoboda11 added a comment.
This revision now requires changes to proceed.

Sorry, I didn't realize that by "next step" you were referring to a future patch. I'd like to see my two comments addressed in this (and a prep) patch, instead of in the future.



================
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));
----------------
Paul-C-Anagnostopoulos wrote:
> 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. 
Great, I think it would be best to do so in this patch.


================
Comment at: llvm/lib/TableGen/TGParser.h:41
+  /// assertion tuple.
   struct RecordsEntry {
     std::unique_ptr<Record> Rec;
----------------
Paul-C-Anagnostopoulos wrote:
> 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.
Can you please do it in a separate patch and rebase this one on top?


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

https://reviews.llvm.org/D99751