-
Notifications
You must be signed in to change notification settings - Fork 36
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
Fix issue #111 on switch/case/default statements. Ensure that the swi… #112
base: master
Are you sure you want to change the base?
Conversation
… that the switch expression is of integer type and apply integer promotion on it. Ensure default statements is not used twice for a given switch. Ensure case values (or ranges for the GCC extension) are integer constants, and that no case value or range is in conflict with the preceding cases of the same switch.
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
==========================================
- Coverage 79.03% 79.01% -0.03%
==========================================
Files 351 351
Lines 46650 46705 +55
==========================================
+ Hits 36872 36906 +34
- Misses 9778 9799 +21
Continue to review full report at Codecov.
|
def add_value(self, val): | ||
""" Add a case value, returns True if OK, False otherwise """ | ||
for low, high in self.ranges: | ||
if val >= low and val <= high: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you know that python has chained comparison logic?
In this case, you could use:
if low <= val <= high:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a neat feature. But I am careful with it because it pushes me to use it in other languages (eg C) where it is syntactically accepted but the result is not what is expected!
self.switch_stack.append(expression.typ) | ||
self.ensure_integer(expression) | ||
prom_expression = self.promote(expression) | ||
self.switch_stack.append(CSwitch(prom_expression.typ)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big improvement, to provide a switch context!
@@ -876,9 +876,10 @@ def parse_switch_statement(self): | |||
self.consume("(") | |||
expression = self.parse_expression() | |||
self.consume(")") | |||
self.semantics.on_switch_enter(expression) | |||
# expression type can be changed by integer promotion | |||
prom_expression = self.semantics.on_switch_enter(expression) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would simply rename this to expression
again. I was confused that it was a promised expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. In fact "prom_expression" was for "promoted_expression"
I used a new name to highlight the fact that the object it references can be different from the object referenced by "expression" (then I added the comment to clarify)
This change looks very well! Could you add some test cases to prevent this bug from re-appearing? I'm sure you ran some test snippets to test this code. I suggest to add those to either test_c.py or otherwise to a |
…tch expression is of integer type and apply integer promotion on it. Ensure default statements is not used twice for a given switch. Ensure case values (or ranges for the GCC extension) are integer constants, and that no case value or range is in conflict with the preceding cases of the same switch.