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

Thu Apr 15 05:16:40 PDT 2021

jansvoboda11 added a comment.

In D99751#2688991 <https://reviews.llvm.org/D99751#2688991>, @Paul-C-Anagnostopoulos wrote:

> In order to use a union in the RecordsEntry struct, I will have to add an enumerated value that specifies which kind of pointer is in the union. Is that worth the bother? Especially since I believe I'll have to write a special destructor.

Would introducing a class hierarchy simplify things?

In D99751#2688868 <https://reviews.llvm.org/D99751#2688868>, @Paul-C-Anagnostopoulos wrote:

> Could I do the union as a prep patch, but please delay the change to a struct for a subsequent patch? This code is an extension to the previous steps that use a tuple. It's easier to think about the change to a struct as a separate patch.

As a reviewer, I'd be much happier approving following patches:

- patch A replaces `std::tuple` with a well-named members (improvement),
- patch B makes `RecordsEntry` represent exactly one of `Record`, `ForeachLoop`, `Assertion` (improvement),
- patch C (this one) enables assertions on multiclasses (improvement).

as opposed to approving this patch that (IMO) exacerbates issues in the existing code, with the promise of future patches that will fix things.

Maybe I'm being too pedantic. Feel free to ping other reviewers, I don't want to block this patch if I'm being unreasonable.

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

https://reviews.llvm.org/D99751