-
-
Notifications
You must be signed in to change notification settings - Fork 788
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
chore: add missing pc maps to vyper_json
output
#3333
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #3333 +/- ##
==========================================
- Coverage 88.93% 88.82% -0.11%
==========================================
Files 84 84
Lines 10606 10608 +2
Branches 2215 2216 +1
==========================================
- Hits 9432 9423 -9
- Misses 768 778 +10
- Partials 406 407 +1
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
vyper/ir/compile_ir.py
Outdated
@@ -755,7 +755,7 @@ def note_line_num(line_number_map, item, pos): | |||
|
|||
def note_breakpoint(line_number_map, item, pos): | |||
# Record line number attached to pos. | |||
if item == "DEBUG": | |||
if item in ("BREAKPOINT", "DEBUG"): |
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.
hmm
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 made this change as the breakpoints were not showing up in the output.
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 did not plan for the new breakpoint instruction to show up in the output. right now pc_breakpoints should be empty
Decompressed pos map is not there tho. Brb, will link what we have to suddenly do in ape-vyper once we switched to vyper-json. |
this is what we had to change to get back to the same data structure we were relying on: https://github.com/ApeWorX/ape-vyper/blob/main/ape_vyper/compiler.py#L241-L263 not the end of the world but i relied on brownie to figure out how to get there, and since the data was already present in the normal way of compiling, it could be added to the |
Understood, I will add the original |
@unparalleled-js I have added |
vyper/compiler/__init__.py
Outdated
"source_map": output.build_source_map_output, | ||
"pc_pos_map": output.build_source_map_output, |
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.
hmm why is this a dup of source_map?
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 wasn't quite sure how to handle pc_pos_map
since it is technically already in the source_map
output. I think what you said below makes sense, so source_map
output will contain both pc_pos_map
and pc_pos_map_compressed
, and there is no need for a separate pc_pos_map
option. Did I understand it right?
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.
right. i'm not sure if we can actually replace the source_map
option since people might depend on it, but we could add it like source_map_full
or something
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.
Hm, so we would still separate it into source_map
-> pc_pos_map_compressed
, and source_map_full
-> pc_pos_map
?
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 think source_map should still return the compressed pc map, but source_map_full should return everything, just like the cli -f source_map
output.
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 mean i think a much better approach is just to not select down into pc_pos_map
for the source_map output. i think source_map
output should already have everything needed.
@@ -38,7 +38,7 @@ def test_keys(): | |||
"breakpoints": data["breakpoints"], | |||
"pcBreakpoints": data["pc_breakpoints"], | |||
"sourceMap": data["source_map"]["pc_pos_map_compressed"], | |||
"pcPosMap": data["source_map"]["pc_pos_map"], | |||
"sourceMapFull": data["source_map"], |
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.
nice. i think we can remove the other new fields now, since they are contained within sourceMapFull.
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.
do we want the other new fields to be sorted?
What I did
Fix #3293 by adding
breakpoints
,pc_breakpoints
anderror_map
-pc_pos_map
is already there so I have skipped that.Additionally, fixed a minor bug with detecting breakpoints.
How I did it
Mostly replicating how it is done for
source_map
.How to verify it
Commit message
Description for the changelog
Add missing pc maps to
vyper_json
outputCute Animal Picture