-
Notifications
You must be signed in to change notification settings - Fork 291
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
Convert series of NET_PACKET_*
defines into a typedef enum
#212
Conversation
|
||
/* Only used for bootstrap nodes */ | ||
#define BOOTSTRAP_INFO_PACKET_ID 240 | ||
/* This type must never exceed a single uint8 */ |
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 think this comment can go away, since it's already present at the bottom, where it's important. If you really must have a comment here, make it say that the enum should be sorted, which forces people to observe the comment at the bottom if they ever think about adding an enumerator > MAX.
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 added that comment to be a 'reminder' that this would be a perfect place for an assert. Maybe we should tackle the use of asserts in toxcore after this?
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.
This is totally not a perfect place for an assert, because an assert would be a syntax error at this point. I'm not sure I understand what you mean. The comment is redundant, and whatever it's a reminder of, is unclear.
NET_PACKET_LAN_DISCOVERY = 33, /* LAN discovery packet ID. */ | ||
|
||
NET_PACKET_DATA_REQUEST = 50, | ||
NET_PACKET_DATA_RESPONSE = 51, |
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.
Where did these two types come from?
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.
They are for #200. Better remove them from this PR.
NET_PACKET_DATA_REQUEST = 50, | ||
NET_PACKET_DATA_RESPONSE = 51, | ||
|
||
/* See: docs/Prevent_Tracking.txt and onion.{c, h} */ |
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.
Can you remove the extra spaces?
/* See: docs/Prevent_Tracking.txt and onion.{c,h} */
0f66929
to
6fdf335
Compare
We've replaced github reviews back to reviewable
Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke. toxcore/network.h, line 119 at r1 (raw file):
|
Reviewed 1 of 1 files at r2. Comments from Reviewable |
@so, and the reason for converting these typedefs to enums is that them having type different from integer Are sizeofs before and after this PR preserved? e.g. |
@nurupo enumeration constants have type |
You are right, not sure what I was even thinking about, it's so obvious too. Reviewed 1 of 1 files at r2. Comments from Reviewable |
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. Comments from Reviewable |
6fdf335
to
97e472b
Compare
Reviewed 1 of 1 files at r2. Comments from Reviewable |
fixup! TravisCI shorten IRC message
97e472b
to
ee3121c
Compare
NET_PACKET_*
defines into a typedef enum
This change is