feat: elide attribute type with new StaticallyTypedAttrInterface
#59
Conversation
ingomueller-net
force-pushed
the
statically-typed-attr
branch
2 times, most recently
from
d3c98d6 to
02f7193
Compare
ingomueller-net
changed the title
StaticallyTypedAttrInterfaceStaticallyTypedAttrInterface [WIP]
ingomueller-net
force-pushed
the
statically-typed-attr
branch
from
02f7193 to
1877e60
Compare
ingomueller-net
changed the title
StaticallyTypedAttrInterface [WIP]StaticallyTypedAttrInterface
ingomueller-net
force-pushed
the
statically-typed-attr
branch
2 times, most recently
from
efadd27 to
007fae0
Compare
ingomueller-net
force-pushed
the
statically-typed-attr
branch
from
007fae0 to
60dcdaa
Compare
|
@dshaaban01: Have a look at this PR. I managed to elide the attribute type from the assembly. I hope we'll be able to merge it soon, so consider starting any new work based on this PR if it's related. |
ingomueller-net
force-pushed
the
statically-typed-attr
branch
from
60dcdaa to
418f69f
Compare
This allows to omit the type of an attribute from the assembly if it can be inferred from the attribute value. For example, the type is redundant in `#substrait.timestamp<100us> : !substrait.timestamp`. The built-in `TypedAttrInterface`, however, forces the appearance of the type in the assembly. The new interface is almost identical but does not enforce it. The PR also makes the two timestamp, the date, and the time attributes implement that interface. Signed-off-by: Ingo Müller <[email protected]>
ingomueller-net
force-pushed
the
statically-typed-attr
branch
from
418f69f to
365e496
Compare
| def Substrait_TimeAttr : Substrait_Attr<"Time", "time", | ||
| [TypedAttrInterface]> { | ||
| def Substrait_TimeAttr : | ||
| Substrait_StaticallyTypedAttr<"Time", "time", "TimeType"> { |
There was a problem hiding this comment.
Micro nit: add additional spaces here to differentiate from body below (I normally just add 2 extra, else this and body looks at same scope ... I should really try out the new formatter)
There was a problem hiding this comment.
Fixed. I tried clang-format, which seems to work on .td files as well, and it works. I have formatted the lines of this commit using it. However, I am unsure as to whether and how to apply or enforce it on the existing files. The diff is quite big and a big commit just for reformatting has disadvantages...
| } | ||
|
|
||
| def Substrait_TimestampAttr : Substrait_Attr<"Timestamp", "timestamp", | ||
| [TypedAttrInterface]> { | ||
| def Substrait_TimestampAttr |
There was a problem hiding this comment.
Nit: colon on this line to match the others.
This allows to omit the type of an attribute from the assembly if it can be inferred from the attribute value. For example, the type is redundant in
#substrait.timestamp<100us> : !substrait.timestamp. The built-inTypedAttrInterface, however, forces the appearance of the type in the assembly. The new interface is almost identical but does not enforce it. The PR also makes the two timestamp, the date, and the time attributes implement that interface.