search

microsoft / cppwinrt

C++/WinRT
MIT License
1.64k stars 238 forks source link

Allow passing array_view as span #1289

Closed JaiganeshKumaran closed 1 year ago

JaiganeshKumaran commented 1 year ago

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

JaiganeshKumaran commented 1 year ago

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

kennykerr commented 1 year ago

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 commented 1 year ago

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

kennykerr commented 1 year ago

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 commented 1 year ago

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

sylveon commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I don't think that #1331 is the best approach, as I indicated in https://github.com/microsoft/cppwinrt/issues/1289#issuecomment-1481317055 - 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jonwis commented 1 year ago

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 commented 1 year ago

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

github-actions[bot] commented 1 year ago

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 commented 1 year ago

@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[bot] commented 1 year ago

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 commented 1 year ago

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)

pjmlp commented 1 year ago

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

osen commented 1 year ago

@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 ;)

pjmlp commented 1 year ago

@osen The lack of respect that paying customers have been dealt with regards to C++/CX deprecation, followed by this decision, proves it was a good decision to refocus on other platforms, away from Windows desktop.

jonwis commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jonwis commented 1 year ago

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:

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 commented 1 year ago

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

github-actions[bot] commented 1 year ago

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 commented 1 year ago

Holy cow this bot is obnoxious.

sylveon commented 1 year ago

I've never liked stale bots.

kennykerr commented 1 year ago

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...

https://github.com/microsoft/microsoft-ui-xaml/issues/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.

https://github.com/microsoft/cppwinrt/blob/9b453cfc518bdaa7e2ff590526c4883457fd6065/.github/workflows/stale.yml#L19-L20

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