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

xdsl: parse dictionary as non-optional #342

Merged
merged 3 commits into from Jan 23, 2023
Merged

Conversation

superlopuh
Copy link
Member

Parse the whole dictionary, including opening and closing braces. This makes a number of things easier to implement, and lets us pass non-optional parsing functions for key and value

@superlopuh superlopuh requested review from math-fehr, mesham, webmiche and georgebisbas and removed request for math-fehr and mesham January 16, 2023 16:04
@superlopuh superlopuh self-assigned this Jan 16, 2023
@@ -356,9 +356,11 @@ def parse_optional_char(self,

def parse_char(self, char: str, skip_white_space: bool = True) -> bool:
assert (len(char) == 1)
current_char = self.get_char()
Copy link
Member Author

Choose a reason for hiding this comment

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

helped me with debugging, might help someone else in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a small docstring/comment on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

My personal opinion is that the code is fairly self-explanatory, I'm not sure what the dosctring would say. Happy to include one if you have a suggestion!

@@ -139,7 +139,7 @@ def get_char(self,
self.skip_white_space()
if self._pos is None:
return None
if self._pos.idx + n >= len(self.str):
if self._pos.idx + n > len(self.str):
Copy link
Member Author

Choose a reason for hiding this comment

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

got an error when parsing the last character of the test inputs

@math-fehr
Copy link
Collaborator

I'll review this once the other one is merged or this PR is rebased, but otherwise this is exactly what we need yes!

Base automatically changed from sasha/lint-printer to main January 16, 2023 21:34
tests/test_parser_error.py Outdated Show resolved Hide resolved
data = parser.parse_dictionary(parser.parse_optional_str_literal,
parser.parse_optional_attribute)
parser.parse_char("}")
data = parser.parse_dictionary(parser.parse_str_literal,
Copy link
Contributor

Choose a reason for hiding this comment

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

good

@@ -356,9 +356,11 @@ def parse_optional_char(self,

def parse_char(self, char: str, skip_white_space: bool = True) -> bool:
assert (len(char) == 1)
current_char = self.get_char()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a small docstring/comment on this?

@superlopuh superlopuh added the xdsl xdsl framework specific changes label Jan 17, 2023
@superlopuh superlopuh changed the title parse dictionary as non-optional xdsl: parse dictionary as non-optional Jan 17, 2023
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 88.44% // Head: 88.73% // Increases project coverage by +0.28% 🎉

Coverage data is based on head (b2762ad) compared to base (01546ab).
Patch coverage: 76.92% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #342      +/-   ##
==========================================
+ Coverage   88.44%   88.73%   +0.28%     
==========================================
  Files          64       64              
  Lines        7815     7864      +49     
  Branches     1281     1286       +5     
==========================================
+ Hits         6912     6978      +66     
+ Misses        645      631      -14     
+ Partials      258      255       -3     
Impacted Files Coverage Δ
xdsl/dialects/builtin.py 81.37% <0.00%> (+0.37%) ⬆️
xdsl/parser.py 84.07% <77.27%> (+0.68%) ⬆️
tests/test_parser.py 100.00% <100.00%> (ø)
tests/test_parser_error.py 100.00% <100.00%> (ø)
tests/test_mlir_printer.py 100.00% <0.00%> (ø)
xdsl/printer.py 95.82% <0.00%> (+0.05%) ⬆️
tests/test_attribute_definition.py 95.36% <0.00%> (+0.75%) ⬆️
xdsl/dialects/memref.py 86.15% <0.00%> (+0.90%) ⬆️
tests/test_printer.py 99.07% <0.00%> (+1.00%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

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.

LGTM!

xdsl/parser.py Outdated Show resolved Hide resolved
@superlopuh superlopuh merged commit b6678ee into main Jan 23, 2023
@superlopuh superlopuh deleted the sasha/dictionary-parsing branch January 24, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
xdsl xdsl framework specific changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants