-
Notifications
You must be signed in to change notification settings - Fork 53
Conversation
macros/support/src/class/class.rs
Outdated
|
||
if ::com::sys::FAILED(hr) { | ||
assert!( | ||
hr == ::com::sys::E_NOINTERFACE || hr == ::com::sys::E_POINTER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI. In theory, E_POINTER would only be returned if the second parameter is null. The caller can guarantee that this is not the case. In practice, implementations will typically AV by dereferencing the pointer without bothering to check since this is a coding error by the caller. Either way, asserting here is not advised as wonky implementations may return some non-standard result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since only E_NOINTERFACE
is expected it probably doesn't make a lot of sense to return Err(hr)
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarijnS95 Hmm I'm not sure what errors would be recoverable from here? I think in general, it's just a question of whether it worked or not, but perhaps you're right that for ultimate flexibility we should be returning an HResult on error so that if some implementation returns a non-standard error, the user can inspect that...
Thanks for re-adding this function, but it is just a copypaste of EDIT: I guess since one class can possibly inherit multiple interfaces this is more troublesome to implement easily? |
@MarijnS95 Deref takes a |
I guess that's a fair point. In the same way that this would segfault if transmuting a |
Going to merge for now. I created an issue for discussing whether |
Fixes issue brought up by @MarijnS95 in #173