-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Adjust Yul CFG export output #15513
Adjust Yul CFG export output #15513
Conversation
a4f1d9d
to
84313bb
Compare
@ekpyron I did a change adding the Yul CFG Json output to the needsHumanTargetedStdout because I believe this was also expected from Alejandro's comment. However, I noticed that we don't use that for the IR AST Json and IR Optimized AST Json export, not sure if we should keep them consistent as well. I updated the comment above since |
We can merge this as is in any case - we just shouldn't forget to check the other case as well :-). I'd need to look into it myself - is there a difference between it being file-based or contract-based or not? The CFG is contract based, an AST isn't, so maybe that's why? But yeah, we should check whether what currently happens makes sense. (But merge this regardless) |
I just checked here and it is actually the IR ast exporter (and optimized version) that does not include such file header. |
b7f7244
to
3548b2d
Compare
84313bb
to
198e2e2
Compare
3548b2d
to
1f20ccd
Compare
198e2e2
to
9ef1aff
Compare
I'm pretty sure it's not that. The AST export was just done by a contributor, who wasn't aware of such details and we didn't want to spend too much effort reviewing it either not to scare them off. Which resulted in lots of forgotten bits like this or bugs like #14452, or the 25% slowdown due to the outputs being naively always generated rather than using |
1f20ccd
to
a7eaac7
Compare
3c1b4fb
to
7972770
Compare
a7eaac7
to
7972c51
Compare
7972770
to
6dd31bb
Compare
Yeah, when I commented, I did so quickly without seeing that it's about the Yul ast, for that it's definitely good/safe to add this as well. |
6dd31bb
to
c38580b
Compare
Depends on #15505
This PR fixes a minor inconsistency in the CLI output of the Yul CFG exporter, which was missing a header when exporting Solidity sources. It also removes the
targets
field from places where this information is irrelevant and adds some tests for the expected output.