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

Convert series of NET_PACKET_* defines into a typedef enum #212

Merged
merged 1 commit into from
Nov 5, 2016

Conversation

GrayHatter
Copy link

@GrayHatter GrayHatter commented Oct 26, 2016

This change is Reviewable

iphydf
iphydf previously requested changes Oct 27, 2016

/* Only used for bootstrap nodes */
#define BOOTSTRAP_INFO_PACKET_ID 240
/* This type must never exceed a single uint8 */
Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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,
Copy link
Member

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?

Copy link
Member

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} */
Copy link
Member

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} */

@GrayHatter GrayHatter dismissed iphydf’s stale review November 2, 2016 06:23

We've replaced github reviews back to reviewable

@GrayHatter
Copy link
Author

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):

Previously, iphydf wrote…

They are for #200. Better remove them from this PR.

Done.

toxcore/network.h, line 121 at r1 (raw file):

Previously, iphydf wrote…

Can you remove the extra spaces?

/* See: docs/Prevent_Tracking.txt and onion.{c,h} */
Done.

Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Nov 2, 2016

:lgtm_strong:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Nov 2, 2016

@so, and the reason for converting these typedefs to enums is that them having type different from integer
can help catch implicit type coercion? Just trying to understand why this PR was made.

Are sizeofs before and after this PR preserved? e.g. sizeof(typedef_value) == sizeof(enum_value). Would be bad if they are not and some code depends on this, especially if such sizeof is used to determine the size of data to throw in a socket, that would make the network protocol incompatible with the one before this PR. Based on the 255 restriction I assume this is not an issue, it probably uses sizeof(uint8_t) or just 1 for sizeofs, but that's just an assumption, it doesn't hurt to double-check.

@iphydf
Copy link
Member

iphydf commented Nov 4, 2016

@nurupo enumeration constants have type int, so that doesn't change anything.

@nurupo
Copy link
Member

nurupo commented Nov 4, 2016

You are right, not sure what I was even thinking about, it's so obvious too.


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Nov 4, 2016

:lgtm_strong:


Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from Reviewable

@JFreegman
Copy link
Member

:lgtm_strong:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from Reviewable

@iphydf iphydf merged commit ee3121c into TokTok:master Nov 5, 2016
@GrayHatter GrayHatter deleted the net_packet_type branch November 19, 2016 07:17
@iphydf iphydf changed the title Convert series of NET_PACKET_* defines into a typedef enum Convert series of NET_PACKET_* defines into a typedef enum Dec 2, 2016
@iphydf iphydf added refactor Refactoring production code, eg. renaming a variable, not affecting semantics and removed code cleanup labels May 4, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
network Network refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants