-
Notifications
You must be signed in to change notification settings - Fork 905
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
htlc.h: move NUM_SIDES to define, not enum member #4209
Conversation
enum side { | ||
LOCAL, | ||
REMOTE, | ||
NUM_SIDES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the intent here is that this is always the last entry in this enum, so that if we ever get more than two sides to a channel (fat chance), we just insert them before the NUM_SIDES
entry. Standard C assigns enum values monotonically IIRC.
With NUM_SIDES
as a #define
you have to insert the new side to the enum
, and update the #define
.
But in any case there is going to be a lot more changes if we ever have more than two sides to a channel so....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this comment as context for this change #4146 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale for not having it as part of the enum
but rather have a #define
is that exhaustive matching requires us to add a case NUM_SIDES
to all switch
statements (and technically if
statements) if we keep it as part of the enum
. +1 for moving it to a #define
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c824623
enum side { | ||
LOCAL, | ||
REMOTE, | ||
NUM_SIDES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale for not having it as part of the enum
but rather have a #define
is that exhaustive matching requires us to add a case NUM_SIDES
to all switch
statements (and technically if
statements) if we keep it as part of the enum
. +1 for moving it to a #define
.
Drive by fixup.
Changelog-None