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

chore(gnolang): revamp switch fallthrough #586

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

tbruyelle
Copy link
Contributor

Description

Found out there's an other TODO CHALLENGE switch fallthrough in pre-process, which makes me realize the proper way of dealing with jump statements like fallthrough.

In the initial version, a new attribute ATTR_SWITCH_CLAUSE_IDX was introduced to store the clause index, but it appears this kind of thing should be handled in the pre-process step.

As a result the attribute is removed and replaced with some code in pre-process step, that stores the clause index in node.BodyIndex, only in case we're dealing with a fallthrough.

How has this been tested?

The tests remain unchanged, which proves this still works as expected.

$ go test ./tests -v -run `/switch'

Found out there's an other `TODO CHALLENGE switch fallthrough` in
pre-process, which makes me realize the proper way of dealing with jump
statements like fallthrough.

In the [initial version][1], a new attribute `ATTR_SWITCH_CLAUSE_IDX` was
introduced to store the clause index, but it appears this kind of thing
should be handled in the pre-process step.

As a result the attribute is removed and replaced with some code in the
pre-process step, that stores the clause index in node.BodyIndex, only in
case we're dealing with a fallthrough.

The tests remain unchanged, which proves this still works as expected.

[1]: gnolang#504
@tbruyelle tbruyelle requested a review from a team as a code owner March 10, 2023 09:41
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Could you write a unit test to verify that the change to the codebase is working as expected and providing the desired outcome?

@tbruyelle
Copy link
Contributor Author

Could you write a unit test to verify that the change to the codebase is working as expected and providing the desired outcome?

The tests I relied on for this change were written in the previous PR #504, they use the tests/files/switch*.gno files. Do you want me to add a test specifically for this package ?

@moul
Copy link
Member

moul commented Mar 12, 2023

Ideally, a test should be conducted to demonstrate the change, such as something that was not functioning properly before. If no new tests can be found, let's use the existing ones.

@tbruyelle
Copy link
Contributor Author

Ideally, a test should be conducted to demonstrate the change, such as something that was not functioning properly before. If no new tests can be found, let's use the existing ones.

Well, the change doesn't add, remove or fix anything. It's just an other way of dealing with fallthrough, but a way that looks more compliant with the existing code. Namely, goto, continue and break use the preprocess step to register the label position, so it seemed normal to me for fallthrough to act this way, instead of using node attributes.

Maybe I should have prefix the change with refac instead of chore, but it doesn't look like a refac for me, since I'm not convinced it brings any performance or clarity improvement. It just looks more compliant, but maybe I'm wrong ?

@moul moul merged commit 81fa834 into gnolang:master Mar 13, 2023
@tbruyelle tbruyelle deleted the chore/switch-fallthrough branch March 16, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

3 participants