Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dialects: (arith) Add custom syntax for arith.constant #1170

Merged
merged 3 commits into from Jun 21, 2023

Conversation

AntonLydike
Copy link
Collaborator

No description provided.

@AntonLydike AntonLydike added dialects Changes on the dialects minor For minor PRs, easy and quick to review, quickly mergeable labels Jun 21, 2023
@AntonLydike AntonLydike self-assigned this Jun 21, 2023
@@ -652,7 +652,7 @@ def _print_attr_string(self, attr_tuple: tuple[str, Attribute]) -> None:
self.print(f'"{attr_tuple[0]}" = ')
self.print_attribute(attr_tuple[1])

def _print_op_attributes(self, attributes: Dict[str, Attribute]) -> None:
def print_op_attributes(self, attributes: Dict[str, Attribute]) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@math-fehr I made this API public so custom ops can print the default attribute dict as well.

Will push another PR to adopt this in the rest of the codebase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1171

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 92.30% and project coverage change: +0.05 🎉

Comparison is base (25c326f) 88.61% compared to head (25637e8) 88.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1170      +/-   ##
==========================================
+ Coverage   88.61%   88.66%   +0.05%     
==========================================
  Files         158      160       +2     
  Lines       22180    22300     +120     
  Branches     3326     3345      +19     
==========================================
+ Hits        19654    19773     +119     
+ Misses       1990     1989       -1     
- Partials      536      538       +2     
Impacted Files Coverage Δ
tests/filecheck/frontend/dialects/scf.py 50.57% <ø> (ø)
tests/test_printer.py 98.22% <ø> (ø)
xdsl/dialects/arith.py 96.71% <90.90%> (-0.51%) ⬇️
tests/test_ir.py 100.00% <100.00%> (ø)
xdsl/printer.py 96.76% <100.00%> (-0.22%) ⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -38,7 +38,14 @@
"cell_type": "code",
"execution_count": 1,
"id": "7aaf5d7c",
"metadata": {},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use --ClearMetadataPreprocessor.enabled=True when you update the notebooks?
That get rid of the metadata

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -652,7 +652,7 @@ def _print_attr_string(self, attr_tuple: tuple[str, Attribute]) -> None:
self.print(f'"{attr_tuple[0]}" = ')
self.print_attribute(attr_tuple[1])

def _print_op_attributes(self, attributes: Dict[str, Attribute]) -> None:
def print_op_attributes(self, attributes: Dict[str, Attribute]) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Copy link
Collaborator

@webmiche webmiche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imagien how much smaller this PR would be if we had used the testing dialect more extensively 😉

@AntonLydike AntonLydike merged commit f86a6ea into main Jun 21, 2023
12 checks passed
@AntonLydike AntonLydike deleted the anton/arith-custom-format branch June 21, 2023 13:19
if isinstance(value, FloatAttr):
typ = value.type
else:
typ = value.typ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tru

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects minor For minor PRs, easy and quick to review, quickly mergeable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants