3 ms·

I'm not sure if second argument is correct either. When assigning through *mut pointer, Drop will be called for previous value, but there's no guarantee that th

I'm not sure if second argument is correct either. When assigning through *mut pointer, Drop will be called for previous value, but there's no guarantee that this value is zero-initialized. (according to https://devblogs.microsoft.com/oldnewthing/20091231-00/?p=15463 https://devblogs.microsoft.com/oldnewthing/20091231-00/?p=15... callee is required to initialize all output arguments, which implies that caller is not required to). It should be represented as &mut std::mem::MaybeUninit<BSTR>

comex 10mo ago
I think you're right. My mistake. Maybe the best option is `&mut windows_sys::core::BSTR` (using the unsafe BSTR type I mentioned), since that way the same BSTR type can be used for the two arguments. Or `*mut BSTR`, since windows-rs itself seems to prefer raw pointers for whatever reason, though I found crates using windows-rs that seem to successfully use references. I am slightly suspicious that Raymond Chen might have been confused. The link in that post has the text "forget to set the output pointer to NULL", but in the linked post (the original link is broken but it's at [1]), the implementation actually set the output pointer to a garbage value rather than leaving it untouched. I wonder what the marshalling implementation actually looks like… But at any rate, treating the out pointer as uninitialized is definitely the safe option. I'm not 100% sure whether it can legitimately point to non-null, but if it does point to non-null, then that value is definitely garbage rather than something that needs freeing. [1] https://devblogs.microsoft.com/oldnewthing/20091007-00/?p=16463 https://devblogs.microsoft.com/oldnewthing/20091007-00/?p=16...
- garaetjjte 10mo ago
  >I think you're right. My mistake. I didn't disagree with you, I just wanted to point another issue. Actually *mut BSTR (owned) is also acceptable, iff you remember to use std::ptr::write instead of normal assignment. > I'm not 100% sure whether it can legitimately point to non-null Note that in none of the examples on this and other posts (like https://devblogs.microsoft.com/oldnewthing/20040326-00/?p=40033 https://devblogs.microsoft.com/oldnewthing/20040326-00/?p=40...) output value is initialized, so it will be whatever is lying on the stack.
  - piker 10mo ago
    These are all great points, and I will update the blog post to reflect them in the morning. I believe this approach can work while retaining the most apparently-idiomatic mapping. What do you guys think? impl IRibbonExtensibility_Impl for Addin_Impl { unsafe fn GetCustomUI(&self, _ribbon_id: BSTR, out: *mut BSTR) -> HRESULT { log("GetCustomUI called()"); std::mem::forget(_ribbon_id); if out.is_null() { return windows::Win32::Foundation::E_POINTER; } unsafe { std::ptr::write(out, BSTR::from(RIBBON_XML)); } S_OK } }
    - garaetjjte 10mo ago
      Looks fine to me, if you're avoiding wrappers like ManuallyDrop/MaybeUninit.
      - piker 10mo ago
        Actually the `windows-rs` team weighed in: impl IRibbonExtensibility_Impl for Addin_Impl { unsafe fn GetCustomUI( &self, _ribbon_id: windows::core::Ref<BSTR>, out: windows::core::OutRef<BSTR>, ) -> HRESULT { log("GetCustomUI called()"); if out.is_null() || out.write(BSTR::from(RIBBON_XML)).is_err() { return E_POINTER; }; S_OK } } https://github.com/microsoft/windows-rs/issues/3832 https://github.com/microsoft/windows-rs/issues/3832 Thanks for pushing on the issue! I've updated the blog post for GetCustomUI.

comex 10mo ago

I think you're right. My mistake. Maybe the best option is `&mut windows_sys::core::BSTR` (using the unsafe BSTR type I mentioned), since that way the same BSTR type can be used for the two arguments. Or `*mut BSTR`, since windows-rs itself seems to prefer raw pointers for whatever reason, though I found crates using windows-rs that seem to successfully use references. I am slightly suspicious that Raymond Chen might have been confused. The link in that post has the text "forget to set the output pointer to NULL", but in the linked post (the original link is broken but it's at [1]), the implementation actually set the output pointer to a garbage value rather than leaving it untouched. I wonder what the marshalling implementation actually looks like… But at any rate, treating the out pointer as uninitialized is definitely the safe option. I'm not 100% sure whether it can legitimately point to non-null, but if it does point to non-null, then that value is definitely garbage rather than something that needs freeing. [1] https://devblogs.microsoft.com/oldnewthing/20091007-00/?p=16463 https://devblogs.microsoft.com/oldnewthing/20091007-00/?p=16...

garaetjjte 10mo ago
>I think you're right. My mistake. I didn't disagree with you, I just wanted to point another issue. Actually *mut BSTR (owned) is also acceptable, iff you remember to use std::ptr::write instead of normal assignment. > I'm not 100% sure whether it can legitimately point to non-null Note that in none of the examples on this and other posts (like https://devblogs.microsoft.com/oldnewthing/20040326-00/?p=40033 https://devblogs.microsoft.com/oldnewthing/20040326-00/?p=40...) output value is initialized, so it will be whatever is lying on the stack.
- piker 10mo ago
  These are all great points, and I will update the blog post to reflect them in the morning. I believe this approach can work while retaining the most apparently-idiomatic mapping. What do you guys think? impl IRibbonExtensibility_Impl for Addin_Impl { unsafe fn GetCustomUI(&self, _ribbon_id: BSTR, out: *mut BSTR) -> HRESULT { log("GetCustomUI called()"); std::mem::forget(_ribbon_id); if out.is_null() { return windows::Win32::Foundation::E_POINTER; } unsafe { std::ptr::write(out, BSTR::from(RIBBON_XML)); } S_OK } }
  - garaetjjte 10mo ago
    Looks fine to me, if you're avoiding wrappers like ManuallyDrop/MaybeUninit.
    - piker 10mo ago
      Actually the `windows-rs` team weighed in: impl IRibbonExtensibility_Impl for Addin_Impl { unsafe fn GetCustomUI( &self, _ribbon_id: windows::core::Ref<BSTR>, out: windows::core::OutRef<BSTR>, ) -> HRESULT { log("GetCustomUI called()"); if out.is_null() || out.write(BSTR::from(RIBBON_XML)).is_err() { return E_POINTER; }; S_OK } } https://github.com/microsoft/windows-rs/issues/3832 https://github.com/microsoft/windows-rs/issues/3832 Thanks for pushing on the issue! I've updated the blog post for GetCustomUI.