Skip to content

box_value constructor: Replace param::hstring with hstring#1530

Merged
DefaultRyan merged 6 commits intomicrosoft:masterfrom
justanotheranonymoususer:box-
Apr 1, 2026
Merged

box_value constructor: Replace param::hstring with hstring#1530
DefaultRyan merged 6 commits intomicrosoft:masterfrom
justanotheranonymoususer:box-

Conversation

@justanotheranonymoususer
Copy link
Copy Markdown
Contributor

Partial fix for #1527.

Other places remain unfixed, example:

unbox_value_or(boxed, ReturnsStringView());

Or any other param::hstring usage.

@github-actions

This comment was marked as resolved.

@justanotheranonymoususer

This comment was marked as resolved.

@github-actions

This comment was marked as resolved.

@justanotheranonymoususer

This comment was marked as resolved.

@github-actions

This comment was marked as resolved.

@justanotheranonymoususer

This comment was marked as resolved.

@github-actions

This comment was marked as resolved.

@sylveon
Copy link
Copy Markdown
Contributor

sylveon commented Mar 20, 2026

@DefaultRyan This PR is a good one - box_value will always do a copy of the hstring so we might as well skip the param::hstring here.

@DefaultRyan DefaultRyan self-assigned this Mar 20, 2026
@DefaultRyan
Copy link
Copy Markdown
Member

This breaks boxing of string-like types such as std::wstring and std::wstring_view via conversion to param::hstring.

I think we can still do this with a little more finesse. One idea of the top of my head is to have an exact hstring overload, and an overload for string-like types that are convertible to param::hstring. For the second overload, we don't actually create a param::hstring, but explicitly construct a fully-fledged hstring instead.

@justanotheranonymoususer
Copy link
Copy Markdown
Contributor Author

I tried to fix, can you run tests?

@DefaultRyan
Copy link
Copy Markdown
Member

I tried to fix, can you run tests?

Did you know you can run the tests locally? That break would have been caught there and saved you some time. :)

@justanotheranonymoususer
Copy link
Copy Markdown
Contributor Author

I was filling lucky. Should pass now, and I even added tests.

@justanotheranonymoususer
Copy link
Copy Markdown
Contributor Author

@DefaultRyan it's green now, are we good?

@justanotheranonymoususer
Copy link
Copy Markdown
Contributor Author

thx for approving, can you also merge?

@DefaultRyan DefaultRyan merged commit 3893ce8 into microsoft:master Apr 1, 2026
82 checks passed
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.

4 participants