windows-rs icon indicating copy to clipboard operation
windows-rs copied to clipboard

More variant types

Open PaulDance opened this issue 1 year ago • 1 comments

Dear maintainers,

This is an attempt to close #2983. It is effectively an upstream of remainder of the equivalent higher-level wrapper we have around VARIANT.

It contains:

  • A constructor for the VT_NULL variant. is_null was also added as a bonus for symmetry with is_empty.
  • From<String> mapping to VT_BSTR since BSTR also implements it.
  • From<&[&str]> mapping to VT_ARRAY | VT_BSTR since BSTR implements From<&str>.
  • From<&[String]> mapping to VT_ARRAY | VT_BSTR since BSTR also implements From<&String>.
  • From<&[u8]> mapping to VT_ARRAY | VT_UI1.
  • Tests for everything. I can always add more if some scenarios were overseen. I actually haven't executed them locally yet in the hope that everything works the first time, so expect them to fail.

The array variants require allocation that is hopefully handled correctly. The Drop implementations were updated accordingly.

Also, is_empty was const, so I thought it would be interesting as a sort of bonus to mark the other methods as such: see first commit. I can remove it though, if it is judged not acceptable.

Cheers, Paul.

PaulDance avatar Jul 04 '24 10:07 PaulDance

@microsoft-github-policy-service agree company="HarfangLab"

PaulDance avatar Jul 04 '24 10:07 PaulDance

Thanks, I'd like to reduce rather than expand VARIANT support in windows-core, likely by moving this to a dedicated crate like windows-variant. I also think that array support should be pushed to a SAFEARRAY wrapper rather than being baked into VARIANT. This is closely tied to VARIANT so I imagine I'd probably combine these in the same crate. At any rate, if you can give me some time to figure out that restructuring it should be easier to get more VARIANT type support up and running.

kennykerr avatar Jul 08 '24 19:07 kennykerr

Alright, ping me when you do :slightly_smiling_face:

PaulDance avatar Jul 09 '24 11:07 PaulDance

?

PaulDance avatar Jul 10 '24 20:07 PaulDance

We'll keep the discussion alive on the issue. 😊

kennykerr avatar Jul 10 '24 21:07 kennykerr

Actually, I just thought about this again: why block this PR's review, iteration and potential merge while waiting for a refactor you want to achieve?

The added APIs would still remain after the refactor, whether they are added before or after, right? Also, in any case, moving the types to a separate crate could potentially constitute an API break, but that is independent of the things proposed here, right?

This and that can be done in parallel, no?

PaulDance avatar Jul 15 '24 17:07 PaulDance