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

Added optional argument on which address to bind (socket). #850

Merged
merged 1 commit into from Oct 14, 2015

Conversation

Projects
None yet
5 participants
@binary1248
Member

binary1248 commented Mar 29, 2015

Supersedes #678.

Should contain everything that was discussed in the previous pull request.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 29, 2015

Member

The resolve function was useful when it returned a single integer that could be assigned in the initializer list of the constructors, but now I think that a private member function would be more readable:

  • no need to return anything, you can directly assign members
  • slightly less code duplication in constructors

The < operator uses m_valid and toInteger(), since it is friend it could use m_adressdirectly for consistency.

Implementing one of the comparison operators using the other would be slightly better than duplicating the logic. Personally I always do that for every comparable class that I write.

Member

LaurentGomila commented Mar 29, 2015

The resolve function was useful when it returned a single integer that could be assigned in the initializer list of the constructors, but now I think that a private member function would be more readable:

  • no need to return anything, you can directly assign members
  • slightly less code duplication in constructors

The < operator uses m_valid and toInteger(), since it is friend it could use m_adressdirectly for consistency.

Implementing one of the comparison operators using the other would be slightly better than duplicating the logic. Personally I always do that for every comparable class that I write.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 29, 2015

Member

Pushed new version. Anything else?

Member

binary1248 commented Mar 29, 2015

Pushed new version. Anything else?

@binary1248 binary1248 added this to the 2.4 milestone Mar 29, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 30, 2015

Member

Shouldn't resolve reset the members if it fails, instead of implicitly keeping their previous values? This would prevent potential mistakes if in the future we call this function in another context.

Member

LaurentGomila commented Mar 30, 2015

Shouldn't resolve reset the members if it fails, instead of implicitly keeping their previous values? This would prevent potential mistakes if in the future we call this function in another context.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 30, 2015

Member

Done. Anything else?

Member

binary1248 commented Mar 30, 2015

Done. Anything else?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 30, 2015

Member

Just nitpicking: the return statements in resolve are not needed anymore. The 3rd one too, if the last piece of code of the function is put in a else block after testing the inet_addr result.

Otherwise, 👍 for me.

Member

LaurentGomila commented Mar 30, 2015

Just nitpicking: the return statements in resolve are not needed anymore. The 3rd one too, if the last piece of code of the function is put in a else block after testing the inet_addr result.

Otherwise, 👍 for me.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 30, 2015

Member

Done.

Member

binary1248 commented Mar 30, 2015

Done.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 16, 2015

Member

Looks fine to me, could need some further testing!

Member

eXpl0it3r commented Sep 16, 2015

Looks fine to me, could need some further testing!

Show outdated Hide outdated src/SFML/Network/IpAddress.cpp Outdated
Show outdated Hide outdated src/SFML/Network/IpAddress.cpp Outdated
Show outdated Hide outdated src/SFML/Network/IpAddress.cpp Outdated
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 16, 2015

Member

Rebased and fixed.

Member

binary1248 commented Sep 16, 2015

Rebased and fixed.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 20, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Sep 20, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Show outdated Hide outdated src/SFML/Network/IpAddress.cpp Outdated
Show outdated Hide outdated include/SFML/Network/IpAddress.hpp Outdated
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 2, 2015

Member

Anything else?

Member

binary1248 commented Oct 2, 2015

Anything else?

@eXpl0it3r eXpl0it3r merged commit 3a12fc6 into master Oct 14, 2015

16 checks passed

sfml-debian-64-gcc Build #350 succeeded in 1 min 27 sec
Details
sfml-osx Build #223 succeeded in 2 min 53 sec
Details
sfml-ubuntu-64-gcc Build #204 succeeded in 1 min 52 sec
Details
sfml-windows7-32-mingw492 Build #286 succeeded in 3 min 31 sec
Details
sfml-windows7-32-msvc10 Build #264 succeeded in 3 min 18 sec
Details
sfml-windows7-32-msvc11 Build #262 succeeded in 4 min 51 sec
Details
sfml-windows7-32-msvc12 Build #256 succeeded in 4 min 8 sec
Details
sfml-windows7-32-tdm471 Build #261 succeeded in 5 min 44 sec
Details
sfml-windows7-32-tdm481 Build #258 succeeded in 3 min 30 sec
Details
sfml-windows7-64-mingw492 Build #269 succeeded in 4 min 20 sec
Details
sfml-windows7-64-msvc10 Build #261 succeeded in 3 min 21 sec
Details
sfml-windows7-64-msvc11 Build #255 succeeded in 5 min 5 sec
Details
sfml-windows7-64-msvc12 Build #275 succeeded in 4 min 7 sec
Details
sfml-windows7-64-tdm471 Build #256 succeeded in 6 min 32 sec
Details
sfml-windows7-64-tdm481 Build #274 succeeded in 4 min 24 sec
Details
test Build #183 succeeded in 1 min 49 sec
Details

@eXpl0it3r eXpl0it3r deleted the feature/network_bind branch Oct 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment