Adding a new network header with associated unit tests #506
Conversation
|
@dunhor FYI: I started over with a new topic branch --> new PR. And now I'm no longer getting clang-format problems, and the winrt COM tests are passing :) |
Sorry for the delayed response; been busy with a bunch of other things at the moment. Regarding clang-format, I took a brief look at your other PR and it looks like you ran clang-format on all files & committed the result. This can have some unintended side effects because different versions of clang-format will format code differently. It's a little unfortunate, but it's not something we have much control over. To help alleviate pains here, we do two things. First, we prefer to use the version of clang-format that ships with Visual Studio for consistency across developers. The second is that the CI machines leverage Regarding the COM tests, it's been failing for a while, but typically passes after one (or two) re-runs. It's on my TODO list for when I get the time to dig into it. Hopefully this can be fixed with something as simple as a detour to make it more deterministic. |
Thanks for the explanation! Yes, this time I just did a git clang-format for edited files. |
|
@dunhor : well, those COM tests decided to start failing after I clang-formatted network.h. :( Any thoughts on how to move this PR forward? can I just force that test to try again? |
I think you should disable them to unblock yourself. Those tests have been far more trouble than they are worth. The original com_timeout PR had me spend about 5-10x as long writing the tests as I did the non-test code. And despite that they still aren't reliable enough. The code they are testing is relatively small and has already been validated so I think that would be ok. Also, I'm unsure if it is interesting that the two failures are x64+Release. x86 isn't failing; Debug is not failing. |
Thanks! |
|
Looks like clang no longer wants to compile this: FAILED: tests/app/CMakeFiles/witest.app.dir/__/FileSystemTests.cpp.obj |
I think it's related to this: The warning -Wdeprecated-literal-operator is now on by default, as this is something that WG21 has shown interest in removing from the language. The result is that anyone who is compiling with -Werror should see this diagnostic. To fix this diagnostic, simply removing the space character from between the operator"" and the user defined literal name will make the source no longer deprecated. This is consistent with CWG2521 https://cplusplus.github.io/CWG/issues/2521.html_. |
Yep - that should be it. |
|
And we get another clang compiler issue: FAILED: tests/app/CMakeFiles/witest.app.dir/__/ResourceTests.cpp.obj |
This apparently is also in the clang release notes: Modified Compiler Flags The -fveclib option has been updated to enable -fno-math-errno for -fveclib=ArmPL and -fveclib=SLEEF. This gives Clang more opportunities to utilize these vector libraries. The behavior for all other vector function libraries remains unchanged. The -Wnontrivial-memcall warning has been added to warn about passing non-trivially-copyable destination parameter to memcpy, memset and similar functions for which it is a documented undefined behavior. It is implied by -Wnontrivial-memaccess |
…r warning nontrivial-memcall.
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Yay, it worked |
| template <typename T> | ||
| explicit socket_address(_In_reads_bytes_(addr_size) const SOCKADDR* addr, T addr_size) WI_NOEXCEPT; |
There was a problem hiding this comment.
Does this need to be a template? Just pass as size_t
There was a problem hiding this comment.
I made it a template since I wanted to avoid unnecessary casting that was likely going to be happening all over.
There was a problem hiding this comment.
The downside to this approach is that this code is now responsible for proper casting. What if someone calls with a negative signed integer? Or a value larger than a size_t can hold? Also, I'm hoping callers are using sizeof when calling, which already has type size_t
There was a problem hiding this comment.
it's handled in the eventual function + they'll get a WI_ASSERT.
template <typename T>
void socket_address::reset(_In_reads_bytes_(addr_size) const SOCKADDR* addr, T addr_size) WI_NOEXCEPT
{
WI_ASSERT(static_cast<size_t>(addr_size) <= static_cast<size_t>(size()));
::memset(&m_sockaddr, 0, size());
if (addr)
{
::memcpy_s(&m_sockaddr, sizeof(m_sockaddr), addr, addr_size);
}
}
There was a problem hiding this comment.
Do you have any example code that would use this constructor/reset function, but not use something like sizeof or something that's not already a size_t? In the tests, I only see two calls to the constructor, both of which use sizeof (though both also use either SOCKADDR_IN or SOCKADDR_IN6, each with their own constructor).
There's also a call to reset in the iterator type, however ai_addrlen is defined as a size_t in all of ADDRINFOA, ADDRINFOW, and ADDRINFOEXW.
Co-authored-by: Duncan Horn <[email protected]>
Co-authored-by: Duncan Horn <[email protected]>
Co-authored-by: Duncan Horn <[email protected]>
Co-authored-by: Duncan Horn <[email protected]>
Co-authored-by: Duncan Horn <[email protected]>
Co-authored-by: Duncan Horn <[email protected]>
Co-authored-by: Duncan Horn <[email protected]>
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // wil::unique_addrinfo resolved_addr; | ||
| // if (0 == GetAddrInfoW(L"name_to_resolve.xyz", nullptr, nullptr, resolved_addr.addressof())) | ||
| // { | ||
| // for (const auto& address : wil::network::addr_info_iterator{resolved_addr.get()}) |
There was a problem hiding this comment.
Wouldn't the typical way looking more like: for (const auto& address : resolved_addr)?
(or for (auto it = resolved_addr.begin(); it != resolved_addr.end(); ++it))
begin and end are typically on the container, not the iterator.
There was a problem hiding this comment.
@dunhor , thoughts?
This is a good point. Maybe I need to create an adapter for the unique_any<> for the wil::unique_addrinfo RAII object (not just managing the raw pointer) - and put begin() and end() on that adapter --> so the range-for loop is given the wil::unique_addrinfo RAII object?
There was a problem hiding this comment.
Or maybe a better/easier option: implement non-member begin and end functions which take a unique_addrinfo as input and return an addr_info_iterator object. (?)
EDIT: hmm. I tried this but I couldn't make it work.
I'm still happy with this syntax for callers:
for (const auto& address : wil::network::addr_info_iterator{resolved_addr.get()})
There was a problem hiding this comment.
Hum, by the time I reached that point of the PR, I forgot this was a simply typedef of a wil::unique_any (but is a conceptually a list).
Having a call to make it an iterator makes sense in that case.
I would likely try to go for a range adapter that takes the unique pointer and implement the begin / end functions instead of having them directly implemented on the iterator type (in the spirit of wil::make_range).
There was a problem hiding this comment.
OK - so is your suggestion would be to have:
for (const auto& address : wil::network::make_range(resolved_addr))
or
for (const auto& address : wil::network::make_range(resolved_addr.get()))
instead of:
for (const auto& address : wil::network::addr_info_iterator{resolved_addr.get()})
?
There was a problem hiding this comment.
There's multiple options you can do here. While you can add begin/end functions for unique_addrinfo, as pointed out, that's just a typedef for unique_any. That means you'd be forcing consumers to either have or construct an unique_addrinfo, which might be inconvenient for them (e.g. if they have a non-owning pointer).
Another option is effectively what you have here: an iterator type that also acts like a range. I call this the std::filesystem::directory_iterator pattern as that type does the same thing. This is a nice pattern that works pretty well when you have a resource or "handle" to a resource (OS handle, filesystem path, registry path, etc.) that, under the right conditions, is enumerable. This pattern fits pretty well here IMO.
Another option is to introduce a "range" type (e.g. addr_info_range) that has begin/end functions. While this is an okay pattern, it can make things like calling STL <algorithm> functions a bit more tedious. E.g. you need a separate line for the range variable in the below:
wil::network::addr_info_range range(addr);
auto itr = std::find_if(range.begin(), range.end(), ...);Of course, if the iterator type is also public/user-facing, consumers could still directly construct the iterators, although that's a less common pattern.
There was a problem hiding this comment.
Thanks for the details, @dunhor.
I wasn't familiar with std::filesystem::directory_iterator, if the pattern is in use in the STL, I don't have any complaints about using it here (and the compatibility with std::ranges is nice).
| socket_extension_function_table_t(const socket_extension_function_table_t& rhs) WI_NOEXCEPT : | ||
| wsa_reference_count{WSAStartup_nothrow()} | ||
| { | ||
| if (!wsa_reference_count || !rhs.wsa_reference_count) | ||
| { | ||
| ::memset(&function_table, 0, sizeof(function_table)); | ||
| } | ||
| else | ||
| { | ||
| ::memcpy_s(&function_table, sizeof(function_table), &rhs.function_table, sizeof(rhs.function_table)); | ||
| } | ||
| } | ||
|
|
||
| socket_extension_function_table_t& operator=(const socket_extension_function_table_t& rhs) WI_NOEXCEPT | ||
| { | ||
| if (!wsa_reference_count || !rhs.wsa_reference_count) | ||
| { | ||
| ::memset(&function_table, 0, sizeof(function_table)); | ||
| } | ||
| else | ||
| { | ||
| ::memcpy_s(&function_table, sizeof(function_table), &rhs.function_table, sizeof(rhs.function_table)); | ||
| } | ||
|
|
||
| return *this; | ||
| } |
There was a problem hiding this comment.
Consider:
Add move constructor/assignment operator that also takes ownership of the unique_wsacleanup_call. This would mostly just buy you one less WSAStartup call (since the function pointers are copied), so not essential, but nice to have.
NOTE: You can technically define these as = default, and they will technically do the correct thing, however the moved-from type will (potentially) still hold non-null pointers in its table. Nobody should be using a moved-from type, so it shouldn't matter, but if they do, might result in tricky to debug issues.
| socket_extension_function_table_t() WI_NOEXCEPT : | ||
| wsa_reference_count{WSAStartup_nothrow()} | ||
| { | ||
| } |
There was a problem hiding this comment.
Consider making this public. As it stands, it's impossible to use this type in a way where initialization gets delayed without either using dynamic allocation or something like std::optional. E.g. if this is a global variable or member of a struct whose lifetime exceeds the (desired) lifetime of the WSAStartup call.
NOTE: If you do move it, you probably want to also move the WSAStartup into load so that a default constructed object doesn't make the call. You'd also probably want a reset function at the very least.
There was a problem hiding this comment.
You'd also probably want to change load such that it "deactivates" wsa_reference_count on failure. If you do that, you can collapse the operator bool into a single implementation that just checks wsa_reference_count.
| if (*this == rhs) | ||
| { | ||
| return false; | ||
| } | ||
| return !(*this < rhs); |
There was a problem hiding this comment.
Is it possible to write something like a compare function (that returns negative for less, zero for equal, and positive for greater)? As-is, you're doing one comparison for the price of two here
Adding a new network header with associated unit tests.