Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Free BSTR/Variant allocated in COMLateBindingObject#setProperty(String, String) #1374

Conversation

matthiasblaesing
Copy link
Member

No description provided.

this.oleMethod(OleAuto.DISPATCH_PROPERTYPUT, null, propertyName, new VARIANT(value));
VARIANT wrappedValue = new VARIANT(value);
try {
this.oleMethod(OleAuto.DISPATCH_PROPERTYPUT, null, propertyName, wrappedValue);
Copy link
Contributor

@dbwiddis dbwiddis Aug 17, 2021

Choose a reason for hiding this comment

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

This change looks good.

However, something to consider. Users may have a similar use case for types VT_DISPATCH and VT_ARRAY.

We already have (next method below) a setProperty(String propertyName, VARIANT value) that does oleMethod() call so we could simply delegate. Or, better, you could define a new overloaded function which users could use an optional boolean parameter similar to OaIdlUtil.toPrimitiveArray()'s destruct parameter or Convert.toJavaObject()'s freeValue parameter:

protected void setProperty(String propertyName, VARIANT value, boolean clear) {
    try {
        setProperty(propertyName, value);
    } finally {
        if (clear) {
            OleAuto.INSTANCE.VariantClear(value);
        }
    }
}

and then simply do

protected void setProperty(String propertyName, String value) {
    setProperty(propertyName, new VARIANT(value), true);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the biggest issue with COM in JNA is the overcomplicated binding for the COM invokers and the VARIANT binding. If the basic interface would be closer to the metal, it would be easier to reason with it.

I would not tinker with a VARIANT I did not create - the user created it, the user can clear it (or not if it is appropriate). The String -> BSTR case is different. In that case we explicitly create a reference counted object (the BSTR) and the user has no control over this.

I saw the Dispatch and IDispatch cases, but also in that case, we just wrap the VARIANT around an existing object without tinkering with the reference count, so the VARIANT is only the structure, nothing more.

Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM. Probably would have done this myself in my last PR, but didn't understand when it was OK to clear the variant.

One suggestion inline.

@matthiasblaesing
Copy link
Member Author

Thanks for the check!

@matthiasblaesing matthiasblaesing merged commit fd6a9a6 into java-native-access:master Aug 18, 2021
@matthiasblaesing matthiasblaesing deleted the com_latebinding_set_property_string branch August 20, 2021 19:02
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.

None yet

2 participants