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

Allow --irOptimized output selection in ethdebug. #15893

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Feb 26, 2025

  • I removed the messages within the soltest tests - having messages there is super annoying in case of changes
  • added more command-line tests
  • the old version only allowed --ir in output selection, that created some confusion because --ir-optimized is always used within the via-ir pipeline, even if optimization is disabled. it will just set minimal optimzation, where the result will be always based on optimized yul - thats why --ir-optimized need to be allowed.

@aarlt aarlt force-pushed the fix_ethdebug_ir_output_selection branch from b11b41b to c729c6a Compare February 26, 2025 17:03
@ekpyron ekpyron requested a review from clonker February 26, 2025 17:25
@aarlt aarlt added the ethdebug label Feb 26, 2025
Copy link
Member

@clonker clonker left a comment

Choose a reason for hiding this comment

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

Mostly style stuff, nothing really wrong with it. Some stuff, especially in the boost testing department could/should be improved but that could also be done in a follow-up post-release - in my opinion at least :).
Perhaps we should (in a follow-up) also think about how to test these flag combinations better. I find it quite hard to parse, what combinations are actually tested and what the expected outcome is. Maybe this could be done in eg a shell script test (with these ~ prefixes) where you have one test for unsupported flag combinations / json inputs which then just asserts that we error out and perhaps check the error message and one test with supported combinations where you just grep if some expected key string is contained in the output. Something like that. Then at least to me it would be clearer what the test coverage actually is and all the relevant stuff is next to each other. Food for thought :)

Copy link
Member

Choose a reason for hiding this comment

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

Same as with the other test, I think this would benefit greatly from using data test cases and consequently unrolling the loops

@@ -0,0 +1,24 @@
{
"language": "Solidity",
Copy link
Member

Choose a reason for hiding this comment

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

Come on, pass it through jq at least :) 1-space indents are just WTF.

Copy link
Member Author

Choose a reason for hiding this comment

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

I totally agree here - not sure what happened 😅

@aarlt aarlt force-pushed the fix_ethdebug_ir_output_selection branch from c729c6a to 4a0037d Compare February 27, 2025 00:47
Copy link
Member

@clonker clonker left a comment

Choose a reason for hiding this comment

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

Just two more little formatting things I found. For the input json files there's also a bit of a mix going on with two-space indents and tab indentations - I am not sure which one is the preferred one :)

@aarlt aarlt force-pushed the fix_ethdebug_ir_output_selection branch from 4a0037d to d2c77ba Compare February 27, 2025 13:46
@aarlt aarlt force-pushed the fix_ethdebug_ir_output_selection branch from d2c77ba to 872805b Compare February 27, 2025 14:05
@aarlt aarlt merged commit 3cfd931 into develop Feb 27, 2025
74 checks passed
@aarlt aarlt deleted the fix_ethdebug_ir_output_selection branch February 27, 2025 16:01
@cameel
Copy link
Member

cameel commented Mar 2, 2025

For the input json files there's also a bit of a mix going on with two-space indents and tab indentations - I am not sure which one is the preferred one :)

There is no situation where we use 1 or 2 space indents other than by mistake. It's always either 4 spaces or a tab in our codebase.

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

Successfully merging this pull request may close these issues.

3 participants