Allow passing array_view as span

[JaiganeshKumaran]

I want to be able to use std::span for portablity and implicitly pass an array_view from WinRT to it.

[JaiganeshKumaran]

@kennykerr Sorry for leaving the issue blank before, please reopen the issue now.

[kennykerr]

The reason the issues page only lets you create a bug report is because cppwinrt is in maintenance mode and no longer receiving new feature work. cppwinrt serves an important and specific role, but further feature development risks destabilizing the project. Additional helpers are regularly contributed to complimentary projects such as https://github.com/microsoft/wil/.

[JaiganeshKumaran]

This is not an external helper, it must be added to the array_view type itself.

[kennykerr]

How std::span is supported is debatable and would ideally just take the place of array_view but that's a non-trivial change.

[JaiganeshKumaran]

You can provide conversion operators although std::span using size_t for size could be an issue as array_view uses uin32_t.

[sylveon]

The reason the issues page only lets you create a bug report is because cppwinrt is in maintenance mode and no longer receiving new feature work.

This is a sad state of affairs considering WinRT is supposed to be the future of Windows development (WASDK & WinUI)

[kennykerr]

This isn't meant as a negative statement. cppwinrt has reached all of its goals and is generally considered complete and largely bug-free (1). Whether WinRT/WinUI/WinAppSDK is the future is debatable. My experience has shown me that the Windows operating system is at its best when you embrace the Windows API as a whole, including Win32/COM/WinRT, and not just the latest shiny wave. You can see this in action with the popularity of projects like win32metadata and windows-rs that support both WinRT and non-WinRT APIs seamlessly.

[JaiganeshKumaran]

We need C++/WinRT to be updated for the win32metadata project to shine for C++, now that C++/Win32 is archived. We need a cleaner way of consuming COM and Win32 APIs without much of the manual HResult handling, and smart pointer noise.

[kennykerr]

I don't think that #1331 is the best approach, as I indicated in #1289 (comment) - array_view was always meant as a bandaid while we waited for span to standardize - but I will reopen the issue for discussion.

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.

https://github.com/orgs/microsoft/teams/cppwinrt-maintainers

[sylveon]

I'm on the side of just removing array_view, and using std::span - but that would mean bumping the minimum to C++20.

[kennykerr]

I'm on the side of just removing array_view, and using std::span - but that would mean bumping the minimum to C++20.

I'd be in favor of that.

[JaiganeshKumaran]

I'm on the side of just removing array_view, and using std::span - but that would mean bumping the minimum to C++20.

Why not simply alias std::span as array_view on C++20, but keep it for C++17?

One thing though is winrt::array_view represents size with an uint32_t and I have some code in C++20 that uses winrt::array_view just because of that, instead of span. I don't see any problem with having implicit conversions, except maybe some template code that requires the type to be identified as a WinRT type and obviously std::span won't in the current arrangement.

[kennykerr]

It is needlessly complicated having types in C++/WinRT that can just as well be served by standard library types. One of the great challenges with the continued maintenance and modernization of C++/WinRT is the growing complexity caused by new features and the baggage of legacy language and library support. Any opportunity for paying back debt is goodness.

[sylveon]

There are enough differences between array_view and span to make this non trivial

[jonwis]

While I'm all for moving people forward to modern language standards, even parts of the OS and associated codebases are still on C++17. Conversions into and out of array_view for std::span when available seems reasonable.

[kennykerr]

Last I heard the OS supported std::span on C++17, but happy for you to make the call.

[github-actions]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

@jonwis is it true that the OS repo supports std::span? Maybe this would allow us to move forward with replacing array_view

[github-actions]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

I wonder how possible it'd be to at least keep source compatibility by typedefing array_view to std::span. The C++ STL doesn't support std::span in C++17, so at least externally this would represent a C++ version bump (unless we keep the old array_view when span isn't available, but this would increase tech debt, not pay it off)

[osen]

@pjmlp

So after deprecating C++/CX, and consequently downgrading the development experience for most of us since 2016, C++/WinRT gets into maintaince mode.

I believe @kennykerr summarized it well with the following:

My experience has shown me that the Windows operating system is at its best when you embrace the Windows API as a whole, including Win32/COM/WinRT, and not just the latest shiny wave

Basically, take anything being "deprecated" with a pinch of salt and don't be too quick to jump onto the latest technique. In other words, Win32 API is still king ;)

[jonwis]

Option A - stymied by ephemerality

This is a semi-common use case of array-passing:

void IFoo::Bar(std::span<uint32_t> arg);
void x(IFoo const& p) {
    p.Bar({1, 2, 3});
}

... which does not work because std::span has no initializer-list constructor. Probably because it can cause UB - if std::span<T> s({T()}); s[0].foo(); compiled it would call a member on a destructed T, as s outlives the initializer_list source. At least in the way we use it today, it's OK-ish; array_view<T> is constructed from an initializer-list and used as an inbound argument in the above snippet, with the expectation that it's passed to the ABI of a call. It's still a Sharp Edge.

See https://gist.github.com/jonwis/2fb5449307399476dabd65f4a95373cc for the example "bad span" and the code showing the hazard.

But, I got pretty far along in the conversion before hitting this. See https://github.com/microsoft/cppwinrt/compare/master...jonwis:cppwinrt:user/jonwis/span-for-arrays?expand=1 as my attempt. It also required updating the baseline to C++20 for span.

Option B - winrt::param::array plus span

Instead, we could consider an winrt::param::array_param<T> type similar to winrt::param::hstring that "just knows" it's being used as in-parameter currency. Projected methods that take arrays as input (like IVector<T>::ReplaceAll) can take array_param that would provide conversions from array<T, N>, vector, T[N], com_array, initializer_list, etc. Anything that can produce a conformant array of less than UINT_MAX elements.

For harness methods that invoke implementations, we could use std::span<T>, initialized from pointer-and-length in the usual way. Callers would have to upgrade their implementations from taking winrt::array_view<T> to std::span<T> ... the downside being that a method like uint32_t FillArray(std::span<T> items); gets more complicated, as the usual auto to_fill = (std::min)(m_myCount, items.size()); /* populate items[0...to_fill]; */ return to_fill; causes a "conversion causes loss of data" as std::span<T>::size() is size_t.

Similarly, com_array<T> must still exist, as it manages the lifetime of a CoTaskMemAlloc'd array of T emitted by methods returning T[].

Option C - retain array-view, add the conversions/constructors

Just merge #1331 so that someone who thinks in std::span can use it when it's available, without changes to the rest. Finish the work by adding tests and ensuring com_array<> can also be constructed from and converted to a span.

Opinion

I think the value-prop of using a specific "this is a winrt array that has certain behaviors and a 32-bit size, but is convertable from a std::span" is the right combination, so my preference is on option C.

[sylveon]

I like the option A as it reduces tech debt (no need to maintain what's pretty much a whole copy of std::span). It is definitely the most breaking of changes, and silently truncating 32 bits sizes isn't ideal either, however.

[kennykerr]

I'm all for reducing technical debt, but option C sounds like the most realistic approach.

[jonwis]

I like the option A as it reduces tech debt (no need to maintain what's pretty much a whole copy of std::span). It is definitely the most breaking of changes, and silently truncating 32 bits sizes isn't ideal either, however.

Sadly "option A" means more words:

p.SetNames(std::array{L"kittens", L"puppies"});

I kind of like "option b" as it makes it super clear that array_view<> is a currency, not a thing you should be using on your own. Nobody (should be) using winrt::param::hstring for anything. Maybe the answer lies between, where the default "empty implementation" projections get updated to use std::span, but array_view is left behind?

I'm all for reducing technical debt, but option C sounds like the most realistic approach.

Agree.

It's kind of a random thread to drop this on, but maybe the answer is to start a wish-list for "C++/WinRT 3.0" ... on my wish list in no particular order:

• C++20 only, upgrade to use all C++20 features where possible
• Automatic fast-ABI for everybody all the time
• Reduce number of QI calls when working with WinRT types compose of many interfaces
• Aggressive detemplatization through more type erasure
• Address compile times
• More easily targetted projections of namespaces (like, "I only want this type, not the rest of it.")
• Hint the compiler on inlining of static methods
• Reduce projected overloads through if constexpr - unless this increases compile times
• Ability to reach back into the std::vector<> inside what single_threaded_vector<> produces.
• More helpers for free-threaded objects
• Simplified property mechanism (like what @asklar was proposing)

Outside of "C++20 only and auto-fast-abi" it's not clear that this is truly a 3.0; most can be done on 2.0 in an additive or nondisruptive way.

[kennykerr]

That was discussed in #1123. Perhaps reopen that issue to avoid your comments being lost when this issue is closed.

[github-actions]

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[jonwis]

Holy cow this bot is obnoxious.

[sylveon]

I've never liked stale bots.

[kennykerr]

It may be a little annoying, but it gives everyone a reminder not to let the issue go unnoticed. By contrast, Xaml has this set to 180 days...

microsoft/microsoft-ui-xaml#8072

...and from experience this just means that 180 days later the customer has given up on the product and the issue is never resolved one way or another.

cppwinrt/.github/workflows/stale.yml

Lines 19 to 20 in 9b453cf

	days-before-stale: 10
	days-before-close: 5

Feel free to tweak these numbers, but I wouldn't recommend bumping them up too high for the reason above.