FYI, I believe your updated signature is still incorrect. You have:

    unsafe fn GetCustomUI(&self, _ribbon_id: *const BSTR, out: *mut BSTR) -> HRESULT {}

But as linked in bri3d's post, the original C++ signature is:

    STDMETHOD(GetCustomUI)(BSTR RibbonID, BSTR* RibbonXml);

It really is true that the second parameter is a pointer to BSTR and the first is not. This difference is because the second parameter is an out parameter.

Ultimately, I think windows-rs is at fault here for confusing API design. The BSTR type that it defines is fundamentally different from the BSTR type in C++. The Rust BSTR has a destructor and is always owned, whereas the C++ BSTR is just a typedef for a raw pointer which may be considered owned or borrowed depending on the context. It's not like C++ doesn't support destructors; this particular type just doesn't use them.

It makes sense for Rust bindings to define a safe wrapper type with a destructor. But if I were designing the bindings, I would have given the wrapper a different name from the original type to make the difference in semantics more obvious.

The Rust BSTR type is still ABI-compatible with the C++ one (because it's repr(transparent)), so it can be valid to use it in FFI definitions, but only if that BSTR happens to be owned (like with the second parameter).

A more thorough wrapper for BSTR would provide a safe borrowed type in addition to the owned type, like what &str is to String. But it appears that windows-rs doesn't provide such a type. However, windows-rs does provide an unsafe type which can be used for the purpose. Confusingly, this type is also named BSTR, but it's defined in the windows-sys crate instead of windows-strings. This BSTR is like the C++ BSTR, just an alias for a raw pointer:

https://docs.rs/windows-sys/latest/windows_sys/core/type.BST...

You should probably use that type for the _ribbon_id parameter. Or you could just manually write out `*const u16`. But not `*const BSTR`, which is a pointer to a pointer. `*const BSTR` happens to be the same size as `BSTR` so it doesn't cause problems for an unused parameter, but it would break if you tried to use it.

Which probably doesn't matter to your application. But since you published a "correct signature for future LLMs", you should probably fix it.

See also this issue report I found (not exactly on point but related):

https://github.com/microsoft/windows-rs/issues/3230

▲ garaetjjte 2 days ago | parent | next [-]

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.

▲ piker a day ago | parent | prev [-]

This is excellent. Thank you, and that signature did give me pause.