sf::IpAddress is always valid, resolution is performed via static member function
#2145
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2145 +/- ##
==========================================
- Coverage 14.06% 14.00% -0.06%
==========================================
Files 190 189 -1
Lines 15879 15868 -11
Branches 4200 4195 -5
==========================================
- Hits 2233 2223 -10
+ Misses 13519 13517 -2
- Partials 127 128 +1
Continue to review full report at Codecov.
|
| @@ -369,7 +369,7 @@ Http::Response Http::sendRequest(const Http::Request& request, Time timeout) | |||
| Response received; | |||
|
|
|||
| // Connect the socket to the host | |||
| if (m_connection.connect(m_host, m_port, timeout) == Socket::Done) | |||
| if (m_connection.connect(m_host.value(), m_port, timeout) == Socket::Done) | |||
There was a problem hiding this comment.
Do we want to check this contains a value to avoid risking a std::bad_optional_access? Maybe just assert that it has a value?
There was a problem hiding this comment.
I think assert would be appropriate here, as far as I understand at this point m_host should be valid.
There was a problem hiding this comment.
Was there ever a conclusion made about this or is it not possible for .value() to be called on a null optional here?
|
SFML/include/SFML/Network/UdpSocket.hpp Line 260 in d7f09c1 Here's another reference to the old |
src/SFML/Network/IpAddress.cpp
Outdated
| // Try to convert the address as a byte representation ("xxx.xxx.xxx.xxx") | ||
| sf::Uint32 ip = inet_addr(address.c_str()); | ||
| if (ip != INADDR_NONE) | ||
| if (const Uint32 ip = inet_addr(address.data()); ip != INADDR_NONE) | ||
| { | ||
| m_address = ip; | ||
| return; | ||
| return IpAddress(NoByteOrderConversionTag{}, ip); | ||
| } | ||
|
|
||
| // Not a valid address, try to convert it as a host name | ||
| addrinfo hints; | ||
| std::memset(&hints, 0, sizeof(hints)); | ||
| addrinfo hints{}; // Zero-initialize | ||
| hints.ai_family = AF_INET; | ||
|
|
||
| addrinfo* result = nullptr; | ||
| if (getaddrinfo(address.c_str(), nullptr, &hints, &result) == 0 && result) | ||
| if (getaddrinfo(address.data(), nullptr, &hints, &result) == 0 && result != nullptr) | ||
| { | ||
| sockaddr_in sin; | ||
| std::memcpy(&sin, result->ai_addr, sizeof(*result->ai_addr)); | ||
| ip = sin.sin_addr.s_addr; | ||
|
|
||
| const Uint32 ip = sin.sin_addr.s_addr; | ||
| freeaddrinfo(result); | ||
| m_address = ip; | ||
| return; | ||
|
|
||
| return IpAddress(NoByteOrderConversionTag{}, ip); | ||
| } |
There was a problem hiding this comment.
Is the overload with NoByteOrderConversionTag necessary? It is only used internally yet requires being part of the externally visible API. Since this functionality is required from within the class itself, couldn't one just create a value that is initialized to e.g. sf::IpAddress::Any and manually set m_address in it? Any compiler that is reasonable at optimizing should be able to combine the statements into a single operation.
vittorioromeo
force-pushed
the
feature/ipaddress_overhaul_pt0
branch
2 times, most recently
from
e433da8
to
5546886
Compare
|
@binary1248: please take one more look, I removed the private constructor. |
vittorioromeo
force-pushed
the
feature/ipaddress_overhaul_pt0
branch
from
5546886
to
c571c13
Compare
|
@binary1248: applied the requested formatting changes. |
Thanks a lot for making a contribution to SFML! 🙂
Before you create the pull request, we ask you to check the follow boxes. (For small changes not everything needs to ticked, but the more the better!)
Description
This PR is related to the issue #2138.
Tasks
How to test this PR?
CI and examples.