Conversation
|
Can you (in this PR or a follow up) update the "LatestBuild" with this new binary? |
| // typename C is the server object implementing IConnectionPoint to Advise() | ||
| // typename S is the client's sink object (must implement type T) | ||
| template <typename T, typename C, typename S> | ||
| void AdviseInProcObject(_In_ Microsoft::WRL::ComPtr<C> sourceObject, _In_ S* connectionSink) |
There was a problem hiding this comment.
Could all the _In_ Foo* pointers be replaced with references? It avoids the need for SAL annotations and let the compiler enforce the `In requirement.
There was a problem hiding this comment.
I don't have a strong opinion between pointer + SAL vs reference. It seems other project in this solution is using pointer + SAL. I'll keep this as is unless you have strong opinion to choose reference.
There was a problem hiding this comment.
Using references is part of the C++ coding guidelines, and since a sample aims to be used as an example, I think it's valuable to have this code to the best standard.
But the choice is up to you, I won't be annoying about this :)
There was a problem hiding this comment.
Would you mind sharing the guidance you're referring to?
There was a problem hiding this comment.
The Cpp core guideline have the following items (not finding one that explicitly recommends reference over pointers, but the intent is pretty clear: have checks, and have them done at compile time rather that run time.
We also have internal guidelines.
| // typename C is the server object implementing IConnectionPoint to Advise() | ||
| // typename S is the client's sink object (must implement type T) | ||
| template <typename T, typename C, typename S> | ||
| void AdviseInProcObject(_In_ Microsoft::WRL::ComPtr<C> sourceObject, _In_ S* connectionSink) |
There was a problem hiding this comment.
Sal not needed on non-pointer types
|
|
||
| using namespace Microsoft::WRL; | ||
|
|
||
| std::wstring ToStringCheckingErrors(const std::wstring& errorString, const std::wstring& dataString) |
There was a problem hiding this comment.
Could be "noexcept" (and constexpr depending on the C++ std version)
| ULONG fetched{0}; | ||
| ComPtr<INetwork> network; | ||
| THROW_IF_FAILED(hr = enumConnectedNetworks->Next(1, network.GetAddressOf(), &fetched)); | ||
| if (hr == S_OK) |
There was a problem hiding this comment.
Why not simply break on S_FALSE instead, and have a "while (true)" loop?
This would make the code simpler (no for loop without increment step, early break) and mostly clarify what success condition other than S_OK is expected there.
There was a problem hiding this comment.
(an alternative, since you use this pattern a lot, could be a "map" function helper that applies a lambda to all the networks in the list)
| SOCKADDR_STORAGE sockAddrStorage{}; | ||
| if (pDestAddr) | ||
| { | ||
| memcpy(&sockAddrStorage, pDestAddr, sizeof(sockAddrStorage) > sizeof(*pDestAddr) ? sizeof(sockAddrStorage) : sizeof(*pDestAddr)); |
There was a problem hiding this comment.
std::max rather than a ternary?
Also, shouldn't it be the min that is needed?
| return returnString.empty() ? std::to_wstring(static_cast<int32_t>(connectivity)) : returnString; | ||
| } | ||
|
|
||
| inline std::wstring ToString(_In_ VARIANT* variant) |
There was a problem hiding this comment.
Could this be a reference?
2 participants
No description provided.