api: constr helpers for generic attributes now mostly static methods #3287
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3287 +/- ##
==========================================
- Coverage 90.04% 90.04% -0.01%
==========================================
Files 446 446
Lines 56239 56248 +9
Branches 5403 5404 +1
==========================================
+ Hits 50639 50647 +8
- Misses 4177 4178 +1
Partials 1423 1423 ☔ View full report in Codecov by Sentry. |
superlopuh
force-pushed
the
sasha/misc/static-constr
branch
from
20cba66 to
9a30300
Compare
xdsl/dialects/stencil.py
Outdated
| BaseAttr[StencilType[Attribute]] | ||
| | ParamAttrConstraint[StencilType[_FieldTypeElement]] | ||
| ): | ||
| if bounds is None and element_type == AnyAttr(): |
There was a problem hiding this comment.
Not specific to here, but what's the benefit to using BaseAttr over a ParamAttrConstraint?
…ctions" This reverts commit 9a30300.
superlopuh
changed the title
|
@math-fehr @alexarice I tried a different approach, which is not to reuse the type parameter on the class definition, seems to work quite well. The only exception is the stencil case, where they actually use type hierarchies, so that bit of the codebase can be a bit more verbose. WDYT? |
| int_attr = prop_def( | ||
| IntegerAttr[IntegerType].constr(type=EqAttrConstraint(IntegerType(16))) | ||
| ) | ||
| int_attr = prop_def(IntegerAttr.constr(type=EqAttrConstraint(IntegerType(16)))) |
There was a problem hiding this comment.
If you wanted to simplify syntax further, you could allow this to take an attribute to be coerced into a Constraint
There was a problem hiding this comment.
I'd rather not go down that route, but it can be done in a different PR
| @@ -1710,23 +1710,22 @@ class ParamOne(ParametrizedAttribute, TypeAttribute, Generic[_T]): | |||
| name = "test.param_one" | |||
| p: ParameterDef[_T] | |||
|
|
|||
| @classmethod | |||
| @staticmethod | |||
There was a problem hiding this comment.
I think this looks good, much closer to what I imagined
There was a problem hiding this comment.
Is there any hope to have all ParametrisedAttributes define this? perhaps with a method defined in @irdl_attr_definition? (Perhaps not as this PR)
There was a problem hiding this comment.
I have a few ideas of where I would start looking for this, and no time in the foreseeable future to actually look into it :(
No description provided.