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: support "fall-through to default" case in switch-over-string #2338

Merged

Conversation

pubiqq
Copy link
Contributor

@pubiqq pubiqq commented Nov 5, 2024

No description provided.

@pubiqq
Copy link
Contributor Author

pubiqq commented Nov 5, 2024

This code definitely needs refactoring, but if I refactored it, it would be impossible to understand what was actually changed to fix the issue, so I left it as it is for now.

@pubiqq pubiqq marked this pull request as draft November 5, 2024 21:00
@skylot
Copy link
Owner

skylot commented Nov 5, 2024

This code definitely needs refactoring

I agree 🤣
I somehow managed to get it working at least for simple case, so yeah code is messy, feel free to refactor it 👍

but if I refactored it, it would be impossible to understand what was actually changed to fix the issue

Well, if you already refactor it and fix the issue it maybe hard to split changes, so it is fine to mix them.

Also, for decompilation tests please provide some checks, at least for containing some string. And for simple tests it is better to add check method (see in other tests), it will be executed on decompiled code to verify result correctness.

@pubiqq pubiqq force-pushed the fallthrough-to-default-in-switch-over-string branch from bf34798 to 83fdb47 Compare November 5, 2024 21:12
@pubiqq pubiqq force-pushed the fallthrough-to-default-in-switch-over-string branch from 83fdb47 to 6696477 Compare November 5, 2024 21:26
@pubiqq pubiqq marked this pull request as ready for review November 5, 2024 21:37
@skylot skylot merged commit 15b6309 into skylot:master Nov 5, 2024
4 checks passed
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.

2 participants