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

Fix issue #111 on switch/case/default statements. Ensure that the swi… #112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tstreiff
Copy link
Contributor

…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.

… 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-commenter
Copy link

codecov-commenter commented Jul 21, 2020

Codecov Report

Merging #112 into master will decrease coverage by 0.02%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
ppci/lang/c/semantics.py 83.47% <78.26%> (-0.46%) ⬇️
ppci/lang/c/parser.py 91.32% <100.00%> (ø)
ppci/arch/mips/instructions.py 86.13% <0.00%> (-1.56%) ⬇️
ppci/arch/riscv/registers.py 93.06% <0.00%> (-0.94%) ⬇️
ppci/binutils/dbg/debugger.py 87.59% <0.00%> (-0.65%) ⬇️
ppci/binutils/dbg/gdb/client.py 53.14% <0.00%> (-0.43%) ⬇️
ppci/wasm/util.py 38.70% <0.00%> (-0.43%) ⬇️
ppci/arch/token.py 95.73% <0.00%> (ø)
ppci/wasm/components.py 81.71% <0.00%> (+0.28%) ⬆️
ppci/lang/tools/lr.py 96.49% <0.00%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e98d2b...31d1070. Read the comment docs.

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:
Copy link
Owner

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:

Copy link
Contributor Author

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))
Copy link
Owner

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)
Copy link
Owner

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.

Copy link
Contributor Author

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)

@windelbouwman
Copy link
Owner

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 C example snippet in the folder test/samples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants