[PATCH] D28942: [globalisel] Re-factor ISel matchers into a hierarchy. NFC

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:124
+template <class Predicate>
+class WithPredicates {
+private:
----------------
ab wrote:
> The name is cryptic ...but I don't have a better idea  ¯\_(ツ)_/¯
> 
> PredicateList? PredicateVector? PredicateHolder?
I'm not entirely sure, but would PredicateMatcher be more in line with the terminology of the other classes used here?
And maybe the class hierarchy becomes a bit more simple to understand if the inheritance went roughly like follows:

```
class PredicateMatcher; /* rename of current WithPredicates. */
class OperandPredicateMatcher : PredicateMatcher<OperandPredicateMatcher>;
class InstructionPredicateMatcher : PredicateMatcher<InstructionPredicateMatcher>;
class OperandMatcher;
class InstructionMatcher;
```
rather than the current

```
class WithPredicates; /* rename of current WithPredicates. */
class OperandPredicateMatcher : PredicateMatcher<OperandPredicateMatcher>;
class InstructionPredicateMatcher : PredicateMatcher<InstructionPredicateMatcher>;
class OperandMatcher : WithPredicates<OperandPredicateMatcher>;
class InstructionMatcher : WithPredicates<InstructionPredicateMatcher>;
```
I'm afraid I haven't had much thought about this, but the first seems a bit more intuitive/easier to understand to me, assuming it's possible at all to define the class hierarchy like that.

Anyway, as I think we're still in the exploring phase for how the tablegen-generator will work, I don't think it's worthwhile to obsess too much on the last polishing touches of the OO-design of this part of the tablegen-generator right now, if it would be delaying further exploration of the tablegen-generator.


https://reviews.llvm.org/D28942