Skip to content

Implementaton of error handling for xlang#361

Merged
manodasanW merged 13 commits into
masterfrom
mawign/error_abi
Apr 27, 2019
Merged

Implementaton of error handling for xlang#361
manodasanW merged 13 commits into
masterfrom
mawign/error_abi

Conversation

@manodasanW

Copy link
Copy Markdown
Member

This change defines the xlang error codes and implements the error origination API and the error info interface exposed in the xlang PAL. Existing code have been updated to use them.

Comment thread src/platform/activation_abi.cpp Outdated
Comment thread src/platform/error_abi.cpp
Comment thread src/platform/published/pal.h Outdated
inline constexpr xlang_result xlang_error_untranslatable_string{ static_cast<int32_t>(0x80070459) };
inline constexpr xlang_result xlang_error_class_not_available{ static_cast<int32_t>(0x80040111) };
inline constexpr int32_t xlang_hresult_no_interface{ static_cast<int32_t>(0x80004002) };
#endif

@Scottj1s Scottj1s Apr 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that we need E_NOINTERFACE for QI, for interop scenarios. But it's a bit confusing that we also have xlang_no_interface. What prevents a QI impl from returning that? Maybe QI should be strongly typed with a com_interop_result enum that contains two values, including this one?

Also, in what scenario would I then use xlang_no_interface?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stance I was thinking of taking is the return value of QueryInterface isn't a xlang error (which would be xlang_error_info*), but rather a convention documented for that function (similar to the return values of AddRef/Release). If the caller of QueryInterface decides that the value they recieved from it is something they consider an error / unexpected, they can then generate an xlang error for it. In the case, QueryInterface returns the hresult version of no interface and that is determined by the caller to be an error in their case, they would originate an xlang error with the error as xlang_result::no_interface.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to complete this PR, but I do want to continue this conversation to determine whether my stance is the right model and whether it would be clear to a developer implementing QueryInterface or whether it should be changed to something as you mentioned or something else.

@Scottj1s

Copy link
Copy Markdown
Member
    virtual int32_t XLANG_CALL QueryInterface(xlang_guid const& id, void** object) XLANG_NOEXCEPT = 0;

consider an enum here to make interop values clear (see below)


Refers to: src/platform/published/pal.h:144 in e14c780. [](commit_id = e14c780, deletion_comment = False)

@manodasanW manodasanW merged commit d2cff46 into master Apr 27, 2019
@manodasanW manodasanW deleted the mawign/error_abi branch April 27, 2019 00:21

private:
xlang_result m_result{};
xlang_string m_message{ nullptr };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these handle types? If so, where is the destructor that releases them?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I will have a PR to address this and your other comment.


void GetMessage(xlang_string* message) noexcept override
{
*message = m_message;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be calling the equivalent of WindowsDuplicateString?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants