core: refactor assembly format directives #3501
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3501 +/- ##
==========================================
+ Coverage 90.32% 90.34% +0.02%
==========================================
Files 464 464
Lines 58043 58047 +4
Branches 5557 5550 -7
==========================================
+ Hits 52427 52443 +16
+ Misses 4188 4177 -11
+ Partials 1428 1427 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| pass | ||
| class VariadicVariable(VariableDirective, AnchorableDirective, ABC): | ||
| def is_present(self, op: IRDLOperation) -> bool: | ||
| return len(getattr(op, self.name)) > 0 |
There was a problem hiding this comment.
| return len(getattr(op, self.name)) > 0 | |
| return bool(getattr(op, self.name)) |
There was a problem hiding this comment.
but then again I wonder whether we should be going via the def instead?
There was a problem hiding this comment.
We have the index of the variable so should be able to do op.operands[self.index]. This should be more type safe too. Should I work that into this PR?
There was a problem hiding this comment.
You couldn't do the above without more refactoring as VariadicVariable doesn't know which type of variable it is
There was a problem hiding this comment.
Ah too bad, although maybe that would also be worth fixing? I'm happy to punt this side quest until someone stumbles upon it and feels motivated
There was a problem hiding this comment.
FWIW this is moved code rather than new code (I don't really see this as an excuse though)
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
| "A variadic region variable cannot be followed by another variadic region variable." | ||
| ) | ||
| case AttrDictDirective(), RegionDirective() if not (a.with_keyword): | ||
| case VariadicOperandDirective(), VariadicOperandDirective(): |
There was a problem hiding this comment.
ah no my bad the diff wasn't showing the input, looks good
| self.raise_error( | ||
| "An `attr-dict' directive without keyword cannot be directly followed by a region variable as it is ambiguous." | ||
| "A variadic operand variable cannot be followed by another variadic operand variable." |
There was a problem hiding this comment.
Glad you've cleaned this up a bit.
Refactors the assembly format directives to: - Remove some unnecessary classes that made the system rigid - Separate directives that go in a `type` clause from format directives
Refactors the assembly format directives to:
typeclause from format directivesThis will make it easier to add
operands,results, andfunctional-typedirectives.