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

More variant types #3146

Closed
wants to merge 9 commits into from

Conversation

PaulDance
Copy link

@PaulDance PaulDance commented Jul 4, 2024

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.

`is_empty` was already marked so but could not be used as such since
there was no way to safely build a value of the type that was indeed
empty. In order to be consistent, mark the rest as `const` too.

Signed-off-by: Paul Mabileau <[email protected]>
@PaulDance
Copy link
Author

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

@kennykerr
Copy link
Collaborator

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
Copy link
Author

Alright, ping me when you do 🙂

@kennykerr kennykerr closed this Jul 9, 2024
@PaulDance
Copy link
Author

?

@kennykerr
Copy link
Collaborator

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

@PaulDance
Copy link
Author

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?

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.

Adding more Variant types
2 participants