microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.35k stars 486 forks source link

More variant types #3146

Closed PaulDance closed 2 months ago

PaulDance commented 3 months ago

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:

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 commented 3 months ago

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

kennykerr commented 2 months ago

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.

PaulDance commented 2 months ago

Alright, ping me when you do :slightly_smiling_face:

PaulDance commented 2 months ago

?

kennykerr commented 2 months ago

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

PaulDance commented 2 months ago

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?