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=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 2 days ago | parent [-]

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=16...

▲ garaetjjte a day ago | parent [-]

>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=40...) output value is initialized, so it will be whatever is lying on the stack.

▲ piker a day ago | parent [-]

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 7 hours ago \| parent [-]
		Looks fine to me, if you're avoiding wrappers like ManuallyDrop/MaybeUninit.