Incorporating xlang error handling into cppxlang#489
Conversation
manodasanW
commented
Jun 28, 2019
manodasanW
commented
- Updating the cppxlang projection to represent the state of error handling in xlang
- Addressing some PR feedback from the previous PR for error handling in xlang with how to represent HRESULT errors from functions whose interface can't change like QueryInterface by introducing com_interop_result
| void GetProjectionIdentifier(xlang_string* projection_identifier) noexcept override | ||
| { | ||
| *projection_identifier = m_projection_identifier; | ||
| *projection_identifier = nullptr; |
There was a problem hiding this comment.
I think you can/should safely assume that copy_to_abi will clear the out param. If that's not the case then we should make it so.
There was a problem hiding this comment.
agreed. Since it has no failure mode, it must always set its out parameter to a stable value.
There was a problem hiding this comment.
copy_to_abi seems to taken in a reference and asserts that the passed reference == nullptr.
| pointer = static_cast<int32_t>(0x80004003) | ||
| }; | ||
| #else | ||
| enum com_interop_result |
There was a problem hiding this comment.
This sounds like a very long-winded way of saying "HRESULT" 😉
| @@ -218,18 +234,18 @@ extern "C" | |||
| #else | |||
| enum xlang_result | |||
There was a problem hiding this comment.
Seems confusing that there both com_results and xlang_results.
There was a problem hiding this comment.
To be clear, I'm not saying there's no merit for this just that we may want to bury the HRESULT in the ABI and just stick with a handful of int32_t values where absolutely necessary (e.g. QI).
There was a problem hiding this comment.
This is a side effect of the long conversation we had about reducing & simplifying the xlang error codes to a new set of values that don't align with the COM values. It's useful to note, then that the ABI for the first three methods is actually COM-compatible including failure values. It's a little verbose but nicely explicit.
There was a problem hiding this comment.
The reason for this change was to avoid confusion on what error is returned here. We have an xlang error for no interface and an COM error for no interface, both of which have different values. To keep compat with COM for IUnknown, we made the choice to keep this as a COM error. If we were to represent that as an int32, it can introduce confusion to a developer on what to return here as the first instinct would be to return / check for the xlang version of no interface as we are in xlang. But, they have to actually return the COM version of the error. To make that clear, the int32 was changed to com_interop_result. I do agree that it is a little verbose, but this should really only affect QueryInterface and related functions.
There was a problem hiding this comment.
Sure, I just think that if anyone is actually implementing QueryInterface and thus having to figure this out they're either one of us or we've failed because nobody should be dealing with the ABI directly.
There was a problem hiding this comment.
I agree no one else should probably be dealing with the ABI directly. This change also makes sure we ourselves don't accidentally use the wrong error value when we return from the com interop functions by essentially having a compile time safety check. In addition, in cross platform scenarios, it ensures that we don't introduce a new com error without explicitly being aware we are adding a new error for interop scenarios and providing a mapping to an xlang error for it.
In theory, we can make returning success less verbose by having a define S_OK or something similar as com_interop_result::success and returning that instead. What do you think about that?
There was a problem hiding this comment.
Macros are evil. Let's leave it as is. 
|
|
||
| AddRef(); | ||
| return 0; | ||
| return com_interop_result::success; |
There was a problem hiding this comment.
This wouldn't be necessary if the return type was simply int32_t. Seems overly verbose for an ABI.
| value{}; | ||
| guid iid{}; | ||
| if (XLANG_IIDFromString(iid_str, &iid) == error_ok) | ||
| if (XLANG_IIDFromString(iid_str, &iid) == 0) |
There was a problem hiding this comment.
Zero here and bla::success below. Zero is so much more concise.
|
Build status not being reflected, but it passed. |
4 participants
