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

Suggested code cleanup by TODO comment #4513

Merged
merged 1 commit into from
May 31, 2021

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented May 6, 2021

This PR (it is not ready there is a new TODO, but I use this to check better the test with Github action) introduces a code cleanup suggested by the TODO comment in the code (I don't know from who, but I guess @cdecker ).

Basically, it moves the code from the if-else statement to a switch statement without the default case. I used the basic idea of the code used in PR #4507.

I'm not sure that this code is better than the code before, also because I'm learning C, so this PR could be a big mistake

Signed-off-by: Vincenzo Palazzo [email protected]

@vincenzopalazzo vincenzopalazzo changed the title Draft: Suggested code cleanup by TODO comment Suggested code cleanup by TODO comment May 6, 2021
@vincenzopalazzo vincenzopalazzo force-pushed the clenup_code branch 2 times, most recently from 9373ebb to 8746bbe Compare May 8, 2021 21:34
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Looking good, just some minor comments on how we could make this even cleaner :-)

wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
@vincenzopalazzo vincenzopalazzo force-pushed the clenup_code branch 2 times, most recently from 77a9e71 to 38b8bd3 Compare May 11, 2021 19:17
@vincenzopalazzo
Copy link
Collaborator Author

Rebase 6dc954b and made changes as suggested.

Thanks, @cdecker to have shown me the trick with the enum.

Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

LGTM as is, my only nit would be to fold all the changes into a single, perfect commit.

wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Outdated Show resolved Hide resolved
wallet/wallet.c Show resolved Hide resolved
wallet/wallet.c Show resolved Hide resolved
@vincenzopalazzo vincenzopalazzo force-pushed the clenup_code branch 3 times, most recently from b86c6c0 to 48f4770 Compare May 26, 2021 19:05
@vincenzopalazzo
Copy link
Collaborator Author

@niftynei Rebased and fold all the commits in only one, sorry for the missing steps.

I'm thinking, to put in the .gitignore the generated files like "wallet/db_postgres_sqlgen.c", but I'm not sure if I'm missing some important steps.

Let's wait for Github actions 🪑

@vincenzopalazzo vincenzopalazzo force-pushed the clenup_code branch 2 times, most recently from 010b1f1 to 30bf4f6 Compare May 30, 2021 09:08
This commit introduces the code cleanup suggested by the TODO comment in the code.

Basically, it moves the code from the if-else statement to a switch statement without the default case. I used the basic idea of the code used in PR ElementsProject#4507.

Changelog-Changed: None.

Signed-off-by: Vincenzo Palazzo <[email protected]>
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack ee98794

@rustyrussell rustyrussell merged commit 0ed7c0d into ElementsProject:master May 31, 2021
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.

4 participants