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

dual_open: correct silent enum conversions #4146

Merged
merged 2 commits into from
Nov 4, 2020

Conversation

darosior
Copy link
Contributor

@darosior darosior commented Oct 22, 2020

We were silently converting a side enum (3 variants) to a tx_role enum
(2 variants).

Signed-off-by: Antoine Poinsot darosior@protonmail.com

Changelog-None

Copy link
Contributor

@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.

Nice. Just needs a small change.

case REMOTE:
role = TX_ACCEPTER;
break;
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, adding a default totally undoes the reason for using a switch statement here, which is to fail to compile if/when the number of enums changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a third alternative (NUM_SIDE), the purpose of this switch was to assert that we are never passed NUM_SIDE (or, as unlikely as it could be, a new alternative value) since it does not make sense for the 2-alternatives enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, NUM_SIDES should be a #define, not within the enum. This is the pattern we used elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked this. NUM_SIDES is a #define, and has been since 2618ef1

channeld/channeld.c Show resolved Hide resolved
@niftynei niftynei force-pushed the fix_silent_conversions branch from 6830218 to aec52a2 Compare October 27, 2020 03:33
Copy link
Contributor

@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.

Updated with suggested changes.

case REMOTE:
role = TX_ACCEPTER;
break;
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked this. NUM_SIDES is a #define, and has been since 2618ef1

@niftynei niftynei force-pushed the fix_silent_conversions branch from aec52a2 to 51e7dcf Compare October 27, 2020 03:35
@niftynei niftynei added this to the v0.9.2 milestone Oct 27, 2020
@cdecker
Copy link
Member

cdecker commented Oct 29, 2020

ACK 51e7dcf

niftynei and others added 2 commits November 1, 2020 14:18
We were silently converting a side enum (3 variants) to a tx_role enum
(2 variants).

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@darosior darosior force-pushed the fix_silent_conversions branch from 51e7dcf to 1c3cb34 Compare November 1, 2020 13:21
@darosior
Copy link
Contributor Author

darosior commented Nov 1, 2020

ACK 1c3cb34

Thanks for taking care of updating this. Had to rebase on master to build properly.

@darosior
Copy link
Contributor Author

darosior commented Nov 1, 2020

Could someone restart https://travis-ci.org/github/ElementsProject/lightning/jobs/740598643 ? The failure seems (very) unrelated and I can no longer do it

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