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

Expose winrt::impl::hstring_builder as winrt::hstring_builder #1035

Closed
lhecker opened this issue Oct 7, 2021 · 15 comments
Closed

Expose winrt::impl::hstring_builder as winrt::hstring_builder #1035

lhecker opened this issue Oct 7, 2021 · 15 comments

Comments

@lhecker
Copy link
Member

lhecker commented Oct 7, 2021

winrt::impl::hstring_builder is a rather useful class. It can be used as the out parameter for various Win32 methods, avoiding the need for extra intermediate copies. The idea of making it public was also suggested in #1018.

Would it be feasible to create a public alias of hstring_builder?

@sylveon
Copy link
Contributor

sylveon commented Oct 7, 2021

I ended up not making it public in my std::format support PR, because it requires some work to make safe for use with std::format. Would rather see that work happen before moving it out of the impl namespace to avoid locking the builder into unsafe by default.

@lhecker
Copy link
Member Author

lhecker commented Oct 7, 2021

Ah sorry. I didn't know it was unsafe... Can you explain what makes winrt::impl::hstring_builder unsafe to use?

@sylveon
Copy link
Contributor

sylveon commented Oct 7, 2021

There's no bounds checking, so something like format_to(back_inserter(builder)) will eventually buffer overflow.

@lhecker
Copy link
Member Author

lhecker commented Oct 7, 2021

Ah okay... yeah but that's fine in this case, isn't it? Member functions called data() almost always return raw pointers including for boundary-checked classes like std::vector. Apart from the finalizing to_hstring(), the hstring_builder has no other member functions. Adding boundary checked iterators to the class in the future should thus be possible I believe.

@kennykerr
Copy link
Collaborator

It can be used as the out parameter for various Win32 methods

Can you give an example? HSTRING is not generally used with Win32 APIs. You're likely better off with something from the standard library.

@lhecker
Copy link
Member Author

lhecker commented Oct 11, 2021

@kennykerr There are a number of Win32 APIs which offer an alternative mode returning the required number of output bytes. For instance CryptBinaryToStringW.
Personally I used it in some fairly disgusting code in combination with ReadProcessMemory here: https://github.com/microsoft/terminal/blob/479ef264b2d9e284db1fc8457f3897d1b19ac32e/src/cascadia/TerminalConnection/ConptyConnection.cpp#L587-L591
Due to the minimalism of the hstring_builder class I believe it's alright to make it public. In my personal opinion there's not much that could break the winrt::hstring_builder API, since that class has not much of an API to begin with. (...which is great!) 😄

@sylveon
Copy link
Contributor

sylveon commented Oct 11, 2021

Yeah the get size then fill pattern is pretty common in C APIs. std::string is even getting support for it soon (resize_and_overwrite)

@kennykerr
Copy link
Collaborator

Thanks for the example. I typically use std::vector for this today but std::string/std::wstring support would be great. I'd like to keep C++/WinRT focused on API binding support and reduce, rather than increase, the number of random helpers that it has to support. Other libraries can and do fill the gaps.

@DHowett
Copy link
Member

DHowett commented Oct 11, 2021

Is there a version of those things that automatically manages an HSTRING_REFERENCE to effect a zero-copy hstring?

@kennykerr
Copy link
Collaborator

@DHowett that's implemented by C++/WinRT automatically.

@sylveon
Copy link
Contributor

sylveon commented Oct 11, 2021

hstring_builder seemingly taps into implementation details of hstring, so my hope by making that public is that we'd have a guarantee that it will work in the future or be updated to follow the implementation, unlike if we where to just copy it within our own project.

@kennykerr
Copy link
Collaborator

There is a public API for this. You can simply call WindowsPreallocateStringBuffer/WindowsPromoteStringBuffer/WindowsDeleteStringBuffer.

@lhecker
Copy link
Member Author

lhecker commented Oct 11, 2021

Are you aware of an alternative API that works on Win7?

@kennykerr
Copy link
Collaborator

No, that's why C++/WinRT inlines all HSTRING support. But again your example doesn't need HSTRING support. You just need to preallocate a buffer which you can easily do today.

@sylveon
Copy link
Contributor

sylveon commented Oct 11, 2021

The reason it's an hstring is because it ends up exposed as a public property:

https://github.com/microsoft/terminal/blob/479ef264b2d9e284db1fc8457f3897d1b19ac32e/src/cascadia/TerminalConnection/ConptyConnection.cpp#L352

But then this isn't really a hotpath (it gets called upon construction). Converting to hstring later sounds fine. And if they really want the best possible performance, then it's possible to wrap around WindowsPreallocateStringBuffer.

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

No branches or pull requests

4 participants