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

[bumbar1]

Before, SFML bound sf::TcpListener to all available addresses internally. This change lets you specify to which address you'd like to bind (listen).

Only been tested on Ubuntu 14.04 (clean rebuild).

[LaurentGomila]

You've read the important comment that you've removed in IpAddress, right? And you have no problem with that?

[eXpl0it3r]

I think it might be good, if one could choose which IP to bind to, but the given implementation is probably not good enough as pointed out by @LaurentGomila.

[Bromeon]

Yes, the obvious alternative to the default parameter would be a separate overload. Then, there is no need for the questionable IpAddress::Any.

[binary1248]

Something like

sf::TcpListener::listenAll(unsigned short port)

would be the simplest solution. Or if we wanted to avoid breaking the current API, something like

sf::TcpListener::listenOn(unsigned short port, const sf::IpAddress& address)

since this is the less common use case.

[bumbar1]

I would leave it as proposed: sf::TcpListener::listen(unsigned short port, const sf::IpAddress& address=_default_), then again I am biased.

About that sf::IpAddress::Any; I agree it's not well thought out. I was thinking listening on sf::IpAddress::None (when defined as INADDR_ANY) wouldn't look sensible, so I had wanted to add another value (sf::IpAddress::Any) to be able to differentiate between them, but that wouldn't be possible because INADDR_NONE == INADDR_BROADCAST.

In the end I believe it would be best to use sf::IpAddress::None as default value (because it's really INADDR_ANY) since I highly doubt anyone would explicitly listen on None.

[MarioLiebisch]

I'd use an overload and use that opportunity to provide the IP address first. To me it's very, very uncommon to provide the port before providing the IP, even if we're not talking about an URI.

[Bromeon]

In the end I believe it would be best to use sf::IpAddress::None as default value (because it's really INADDR_ANY) since I highly doubt anyone would explicitly listen on None.

Why would you prefer a default parameter to an overload here? Default parameters make sense when they improve the expressiveness of the API, i.e. when the caller knows what value is used if he omits the argument. Here, the opposite is the case: sf::IpAddress::None is not intuitive, the caller must read the documentation to know what it means.

I'd use an overload and use that opportunity to provide the IP address first

I would provide the IP address last, because it's a common idiom to move parameters that represent additions to other overloads to the end. I've also heard the expression "listen on a port" more often, and e.g. the Java ServerSocket constructor does it the same way.

[bumbar1]

@Bromeon as you've stated in your first commend, separate overload would indeed be the simplest, which is something this library strives for.

Refactor the current listen method into a helper private one, which takes port and address as parameters.

[binary1248]

So... are there any remaining issues with this pull request? @bumbar1's revised version corresponds to what came out of the discussion and should be easy to review within a few minutes.

Edit: Maybe renaming the ip parameter of listen to address would make more sense, but that's just me...

[LaurentGomila]

Edit: Maybe renaming the ip parameter of listen to address would make more sense, but that's just me...

Yes, it should be named "address".

[bumbar1]

I've renamed parameter and updated \see reference.

[binary1248]

So... is there still anything preventing this pull request from being accepted and merged?

[LaurentGomila]

The order (port, address) feels weird, everything else uses (address, port). But given that address is the optional parameter, it also makes sense in this order. So... I don't know, but I'm not fully satisfied with this API.

Using IpAddress::None feels weird too, because you're supposed to bind to any address, not to none... It's correct because IpAddress::None internally equals ADDRESS_ANY, but it's very confusing from a user point of view.

[binary1248]

The order (port, address) feels weird, everything else uses (address, port).

It might seem weird, but if you consider that we are on the "receiving end" of the connection, addresses (the port is also considered an address in the transport layer) are interpreted in reverse order of the sending side. If you ask me, both make little sense 😏. It would make more sense for the sender to connect using port first then address, since the TCP PDU has to be assembled before wrapping it inside an IP packet, which is where the address comes into play. If, theoretically, SFML would support multicast, the port would come first as well, and the function would naturally fit into that ordering.

What I am trying to say is that there really isn't any convention that "makes sense". Just use whatever makes the most sense from a functional point of view. So, I don't see any issue with (port, address) to be honest.

because you're supposed to bind to any address, not to none

Technically, you actually bind to no address i.e. you are not bound to a specific address, meaning incoming connections on interfaces regardless of the address(es) bound to them should be handled by the socket since it isn't bound to a specific address. Think of the address like a filter for the kernel. If there is no filter, you get everything. From that perspective, IpAddress::None is correct, and for me also "feels right". People who have gotten accustomed to other APIs might already be used to binding to no address to accept connections on all interfaces, and the people who come from a theoretical background would understand it anyway.

[LaurentGomila]

What I am trying to say is that there really isn't any convention that "makes sense".

Yes, but it's still inconsistent with the other functions. And you know that I don't like inconsistencies.

Technically, you actually bind to no address

I guess it depends on the point of view. "Any" looks more correct to me, since the system is allowed to pick any network interface, not none. "Any" means "I don't care", which is exactly what happens. And at the BSD socket level, it's INADDR_ANY too, not INADDR_NONE.

[bumbar1]

I guess it depends on the point of view. "Any" looks more correct to me, since the system is allowed to pick any network interface, not none. "Any" means "I don't care", which is exactly what happens. And at the BSD socket level, it's INADDR_ANY too, not INADDR_NONE.

That's why I wanted IpAddress::Any but having Any == None feels wrong. I don't see a problem with calling listen with None because it's internal; user doesn't really see it -- unless going through source, but then they'll see that None is really INADDR_ANY and I think they'll understand.

Having address as second argument to listen may seem counter intuitive (looking at connect functions for example), both seem fine; having port first may feel it's breaking current API less (if that makes sense) and having address first feel more intuitive.

[binary1248]

Yes, but it's still inconsistent with the other functions. And you know that I don't like inconsistencies.

Well... I guess listen could also take the address first then port. I would be willing to sacrifice the optional parameter last for the consistency with the rest of the API.

I guess it depends on the point of view. "Any" looks more correct to me, since the system is allowed to pick any network interface, not none. "Any" means "I don't care", which is exactly what happens. And at the BSD socket level, it's INADDR_ANY too, not INADDR_NONE.

What about sf::IpAddress::All? :stuck_out_tongue:
It would fit nicer to prose text as well: "Listen on all addresses" instead of "Listen on any address". The latter sounds too much like you could pick one.

[LaurentGomila]

I don't see a problem with calling listen with None because it's internal

It's not only internal, code like this can exist:

sf::IpAddress interface = sf::IpAddress::None; // <-- here
if (some_condition)
    interface = sf::IpAddress(some_specific_ip);
socket.listen(port, interface);

Well... I guess listen could also take the address first then port. I would be willing to sacrifice the optional parameter last for the consistency with the rest of the API.

I guess we need at least a third opinion then.

What about sf::IpAddress::All?

What about sf::IpAddress::Any, and add a m_valid boolean parameter to sf::IpAddress instead of trying to hijack the IP value, for which the full range has a potential different meaning?

I know it's not our philisophy, but for the network module, I feel like differing slightly from the BSD socket API could be confusing for some people.

[bumbar1]

Third overload would then be listen(IpAddress& address, unsigned short port) and all except sf::IpAddress::Any (which would be INADDR_ANY, same as sf::IpAddress::None) constants would have a member m_valid set to true, correct?

If so, beside adjusting comparison operators, to account for the new member, would there be anything else to add/modify?

[LaurentGomila]

Third overload

You mean second, right?

all except sf::IpAddress::Any (which would be INADDR_ANY, same as sf::IpAddress::None) constants would have a member m_valid set to true, correct?

All except sf::IpAddress::None, which would not have a specific value. Then, since its value is undefined, all operators and functions of sf::IpAddress would have to check the m_valid member before doing anything else.

[bumbar1]

I thought three overloads would be listen(port), listen(port, address) and listen(address, port) to please everyone. :)

[Bromeon]

The order (port, address) feels weird, everything else uses (address, port). But given that address is the optional parameter, it also makes sense in this order. So... I don't know, but I'm not fully satisfied with this API.

Good point. In that case, (address, port) may indeed be the better order. Later, it would also be possible to rename the unary listen() overload to something more expressive...

I thought three overloads would be listen(port), listen(port, address) and listen(address, port) to please everyone. :)

Our intent is never to please everyone, and duplicating functionality for the sake of it just leads to a bloated and hard-to-understand API.

[LaurentGomila]

I thought three overloads would be listen(port), listen(port, address) and listen(address, port) to please everyone. :)

An overload that only changes the order of arguments is the worst thing I've ever seen (fortunately I haven't seen it yet in a library :p).

[LaurentGomila]

Sounds good to me as a workaround, to be replaced by exceptions when the time's right. ;)

Ok, ok :p

[bumbar1]

Is there a reason why IpAddress comparison operators are not friends? Problem now is that adding m_valid member requires a public method to access it, isValid(), and it can confuse users, for example:

sf::IpAddress addr("256.256.256.256");
if (addr.isValid()) {
    // this is true, because only default constructor sets m_valid to false
}

[LaurentGomila]

Is there a reason why IpAddress comparison operators are not friends?

Because they didn't need to. So obviously this can be changed if they now need to access a new private member.

But your example is wrong, any invalid address should have m_valid to false, not just default constructed ones.

[bumbar1]

If m_valid is to be exposed publicly through isValid() method, then there's no need to make IpAddress comparison operators friends.

Should UdpSocket::bind have a default IpAddress::Any as well?

[LaurentGomila]

If m_valid is to be exposed publicly through isValid() method

I never said it was ;)

Should UdpSocket::bind have a default IpAddress::Any as well?

It would make sense, yes.

[bumbar1]

Should m_valid have a public get method, that users can rely on? Should all functions/methods (eg send, connect, listen) check if address is valid?

[LaurentGomila]

We don't have to change anything in the public API, sf::IpAddress::None can still be used for invalid addresses, except that internally it would have the m_valid flag unset instead of using a special address value.

[Bromeon]

What do all those functions do if they are passed an invalid address? Nothing (i.e. hide logic error)? Assertion? sf::err()?

I'm talking about now, where we have no exceptions.

[LaurentGomila]

What do all those functions do if they are passed an invalid address? Nothing (i.e. hide logic error)? Assertion? sf::err()?

This is a different problem, that already exists with the current code.

[eXpl0it3r]

Am I right in assuming that this PR isn't fully ready yet?

[LaurentGomila]

Yes.

[bumbar1]

What's the state of this PR?

[Bromeon]

Why did you move the operators to members? That has nothing to do with the feature, and there's a good reason why binary operators are defined globally...

[bumbar1]

Because they need access to m_valid member and @LaurentGomila said there's no need to add isValid() member function.

[binary1248]

You didn't consider that toInteger() would need to be changed? 😉 Because as long as the integer representations of both addresses are equal, the addresses can be considered equal as well, which is pretty much how the equality operator works now...

Also, if resolve() fails to find a valid address, you would still set m_valid to true in 2 constructors. I don't think this makes much sense...

[bumbar1]

How should invalid and any address be represented with only 0 available?

That's because m_valid is only used to differentiate between any and invalid. It doesn't actually tell if it's a valid IPv4 address or not (hence not adding isValid method to public API, since it would confuse users).

[binary1248]

Superseded by #850.