Skip to content

Conversation

@charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Dec 16, 2024

What I did

refactor venom tests with new parsing machinery. it should make tests easier to read. add an example of the new testing style

How I did it

How to verify it

Commit message

refactor venom tests with new parsing machinery. it should make tests
easier to read. refactor `test_simplify_cfg.py` as an example of the
new testing style

additionally add some parser / text format fixes:

- switch order of operands in `phi` text format
- fix `jnz` parsing to match `IRInstruction.__repr__()`
- add comments to venom grammar (`;` indicates a comment a la llvm)
- make data section optional
- fix grammar rule: expr can be any operand.

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 50.92%. Comparing base (537313b) to head (b12f75e).

Files with missing lines Patch % Lines
vyper/venom/parser.py 0.00% 6 Missing ⚠️
vyper/venom/basicblock.py 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (537313b) and HEAD (b12f75e). Click for more details.

HEAD has 183 uploads less than BASE
Flag BASE (537313b) HEAD (b12f75e)
200 17
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4401       +/-   ##
===========================================
- Coverage   91.11%   50.92%   -40.20%     
===========================================
  Files         115      115               
  Lines       16225    16227        +2     
  Branches     2728     2729        +1     
===========================================
- Hits        14784     8264     -6520     
- Misses       1006     7345     +6339     
- Partials      435      618      +183     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

s += opcode
operands = self.operands
if opcode not in ["jmp", "jnz", "invoke"]:
if opcode not in ("jmp", "jnz", "invoke"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I now this is not necessarily part of the PR but there it should be self.opcode no? since the opcode contains space after original opcode so body of this if is unreachable right now (I have noticed it when thinking about order of operands in jnz)

Copy link
Member Author

Choose a reason for hiding this comment

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

yea seems so. let's leave it out of scope, can be fixed in #4402 or #4403

@charles-cooper charles-cooper merged commit 135c2d6 into vyperlang:master Dec 17, 2024
157 checks passed
@charles-cooper charles-cooper deleted the refactor/venom-tests branch December 17, 2024 14:12
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