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

Transfer the client_port to SocketDescriptor #15

Merged
merged 6 commits into from
Mar 26, 2017
Merged

Conversation

B3n30
Copy link
Owner

@B3n30 B3n30 commented Mar 25, 2017

No description provided.

@@ -175,8 +175,7 @@ void RoomMember::ReceiveLoop() {
};

void RoomMember::Join(const std::string& nickname, const std::string& server, uint16_t server_port, uint16_t client_port) {
// TODO(Subv): Use client_port.
RakNet::SocketDescriptor socket;
RakNet::SocketDescriptor socket(client_port,"");
Copy link
Collaborator

Choose a reason for hiding this comment

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

space after the comma

@Subv
Copy link
Collaborator

Subv commented Mar 25, 2017

Does this actually work? what is that client port used for?

@B3n30
Copy link
Owner Author

B3n30 commented Mar 25, 2017

As stated in the RakNet documentation: The local port to bind to. Pass 0 to have the OS autoassign a port.

@@ -175,7 +175,7 @@ void RoomMember::ReceiveLoop() {
};

void RoomMember::Join(const std::string& nickname, const std::string& server, uint16_t server_port, uint16_t client_port) {
RakNet::SocketDescriptor socket(client_port,"");
RakNet::SocketDescriptor socket(client_port, "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of the sample code uses 0 as second argument btw.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I recognized that too. But in the documentation it states:
SocketDescriptor(unsigned short _port, const char *_hostAddress);
... Pass an empty string to use INADDR_ANY.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I just checked the RakNet source code, and apparently 0 is the right way...

@@ -175,8 +175,7 @@ void RoomMember::ReceiveLoop() {
};

void RoomMember::Join(const std::string& nickname, const std::string& server, uint16_t server_port, uint16_t client_port) {
// TODO(Subv): Use client_port.
RakNet::SocketDescriptor socket;
RakNet::SocketDescriptor socket(client_port, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use nullptr instead of 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is how it appears all over raknet source, I think we should keep as is and process them all together later [we can create an issue for it?]

Copy link
Owner Author

Choose a reason for hiding this comment

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

So internally RakNet checks if (hostAddress) /* With host address the second argument */
if false, it will assign hostAddress[0] = 0. Thus if hostAddress=nullptr this will fail...

We should stay with 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

nullptr IS the same as 0 in this case. nullptr is (afaik) meant as a drop in replacement for NULL which is defined as #define NULL (void*)0.

However, I feel like we can go through and fix these later because we have so many other occurrences in the codebase. It's one of these: Don't turn a "haystack made out of needles" into "a haystack made of hay with just one needle".

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thats right, but RakNet tries to set the value 0 to the memory area the nullptr is pointing to. Which will fail with a BAD_ACCESS error...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then it would also fail with the current code.
Note that it uses _hostAddress (the argument) to read from but moves to hostAddress (no underscore) which is the member variable.

Copy link
Collaborator

@Subv Subv left a comment

Choose a reason for hiding this comment

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

I don't agree with the nullptr->0 change, but whatever, lets get this over with.
Squash it and merge it please

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.

3 participants