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

Allow getting array_view/span out of IBuffer and IMemoryBuffer #360

Open
JaiganeshKumaran opened this issue Sep 2, 2023 · 6 comments · May be fixed by #359
Open

Allow getting array_view/span out of IBuffer and IMemoryBuffer #360

JaiganeshKumaran opened this issue Sep 2, 2023 · 6 comments · May be fixed by #359
Labels
feature-request New feature or request

Comments

@JaiganeshKumaran
Copy link

JaiganeshKumaran commented Sep 2, 2023

For safety, the conversion operator can be explicit.

@kennykerr
Copy link
Contributor

I've tagged a few project maintainers. If one of them is available to offer guidance and mentorship for this issue, they will reach out according to the contributing guide to discuss and agree on an approach.

@microsoft/cppwinrt-maintainers

@jonwis
Copy link
Member

jonwis commented Sep 3, 2023

@JaiganeshKumaran can you write a little sample code showing what you'd like to see?

@oldnewthing
Copy link
Member

oldnewthing commented Sep 6, 2023

template <typename D>
struct consume_Windows_Foundation_IMemoryBufferReference
{
   /* existing code unchanged */

    auto view() const
    {
        uint8_t* data{};
        uint32_t capacity{};
        check_hresult(static_cast<D const&>(*this).template as<IMemoryBufferByteAccess>()->GetBuffer(&data, &capacity));
        return array_view(data, capacity);
    }
};

template <typename D>
struct consume_Windows_Storage_Streams_IBuffer
{
   /* existing code unchanged */

    auto view() const
    {
        uint8_t* data{};
        check_hresult(static_cast<D const&>(*this).template as<IBufferByteAccess>()->Buffer(&data));
        return array_view(data, Length());
    }
};

Now you can write

IBuffer buffer = get_some_buffer_from_somewhere();
auto view = buffer.view(); // returns an array_view<uint8_t>

IMemoryBufferReference ref = memorybuffer.CreateReference();
auto view = ref.view(); // returns an array_view<uint8_t>

This is much safer than using data(), which gives you a pointer to the start of the data but no information about how much data there is.

@JaiganeshKumaran
Copy link
Author

JaiganeshKumaran commented Sep 10, 2023

Not really a fan of having a separate view member function - it is not how the standard library does things. An implicit conversion operator also has the advantage of passing an IBuffer directly to a function taking array_view<uint8_t const> (pass-array in WinRT). Now for safety, the rvalue-qualified conversion operator should be deleted. This does, however, prevent some valid code eg: FunctionTakingArrayView(FunctionReturningBuffer()).

Another thing to consider is allowing to take a std::span<std::byte> from buffers, in addition to array_view/span<uint8_t>.

@sylveon
Copy link
Contributor

sylveon commented Sep 10, 2023

I don't think it should be deleted, as you've shown there are valid usages of r-values.

@oldnewthing
Copy link
Member

If the operator is explicit, then it actually gets worse because you have to perform the wordy conversion explicitly.

  • With explicit conversion: FunctionTakingArrayView(array_view<uint8_t>(FunctionReturningBuffer())). (You don't get any help from CTAD here. FunctionTakingArrayView(array_view(FunctionReturningBuffer())) does not compile.)
  • With view member: FunctionTakingArrayView(FunctionReturningBuffer().view())).

The explicit conversion from hstring to wstring_view suffers from this same problem.

@kennykerr kennykerr transferred this issue from microsoft/cppwinrt Sep 18, 2023
@dunhor dunhor added the feature-request New feature or request label Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants