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.

Sign up for GitHub

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

Jump to bottom

More variant types #3146

Closed
wants to merge 9 commits into from
Closed

More variant types #3146

wants to merge 9 commits into from

Conversation

PaulDance
Copy link

@PaulDance PaulDance commented Jul 4, 2024 •
edited
Loading

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
Feat(core/variant): Mark all methods as const
8e7fb55 
`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
Feat(core/variant): Add new null and is_null methods
a3fa6e8 
Signed-off-by: Paul Mabileau <[email protected]>
@PaulDance
Test(core/variant): Cover new null and is_null methods
2af5c1d 
Signed-off-by: Paul Mabileau <[email protected]>
@PaulDance
Feat(core/variant): Add From<String> implementations
04636e9 
Signed-off-by: Paul Mabileau <[email protected]>
@PaulDance
Test(core/variant): Cover From<String> implementations
d39f56f 
Signed-off-by: Paul Mabileau <[email protected]>
@PaulDance
Feat(core/variant): Add From<&[&str|String]> implementations
12fdbfe 
Signed-off-by: Paul Mabileau <[email protected]>
@PaulDance
Test(core/variant): Cover From<&[&str|String]> implementations
4aa9a25 
Signed-off-by: Paul Mabileau <[email protected]>
@PaulDance
Feat(core/variant): Add From<&[u8]> implementations
dbde494 
Signed-off-by: Paul Mabileau <[email protected]>
@PaulDance
Test(core/variant): Cover From<&[u8]> implementations
4ed925f 
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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding more Variant types
2 participants
@PaulDance @kennykerr