C++20 ranges support

[sylveon]

Follow-up to #873
Fixes #807

The regression was caused by me relying on some C++20 changes that automatically defined pointer to void in iterator_traits if it wasn't defined by the iterator, while C++17 didn't define any member when that happened. Resolved by manually defining pointer to void (which is the most appropriate thing here, since WinRT iterators don't expose operator->). With this change the whole test suite builds, runs, and passes.

However, I am getting a lot of 'winrt::impl::consume_Windows_Foundation_Collections_IIterable<D,T>::begin': unreferenced local function has been removed warnings when building, and I'm not sure where they come from, since the way this is implemented is 1:1 to things like wait_for on IAsyncOperation, and that function doesn't exhibit the issue. Any idea what causes this?

[kennykerr]

Hmm, its not immediately obvious what those warnings are trying to tell us. 😜

[kennykerr]

Build is good, apart from the warnings.

[sylveon]

I reproduced the warning: it occurs when the function is used in unevaluated contexts, but never used outside of that, for example this code triggers it

auto v = winrt::single_threaded_vector<int>();
decltype(v.begin()) a;
// uncomment to make warning disappear
// v.begin();

It's not specific to this change however, for example the following triggers the same warning:

winrt::Windows::ApplicationModel::AppDisplayInfo a(nullptr);
decltype(a.Description()) b;

Creating a fast_iterator without actually using it also shows the issue, because GetAt() is used unevaluated to detect if fast_iterator can be used but it's never actually used:

auto v = winrt::single_threaded_vector<int>();
begin(v);

The code above will trigger the warning

1>T:\Projects\cppwinrt\_build\x64\Debug\winrt\impl\Windows.Foundation.Collections.0.h(438,28): warning C4505: 'winrt::impl::consume_Windows_Foundation_Collections_IVector<winrt::Windows::Foundation::Collections::IVector<int>,T>::GetAt': unreferenced local function has been removed
1>        with
1>        [
1>            T=int
1>        ]

I don't think there's anything I can do to easily fix it :(
Declaring the methods the compiler warns about as inline gets rid of the warning, but to properly resolve it then we'd have to go and mark every function from the consume_* types (both autogenerated and manually written) to be inline as well, which is very much out of scope for this PR.

Other than that, disabling the warning obviously works, as it's basically just noise (IMO it should be moved out of /W4 to /Wall).

[kennykerr]

auto v = winrt::single_threaded_vector<int>();
begin(v);

This sort of thing seems like a bug - if a test is doing this and causing a warning I'd be ok with fixing the test.

[sylveon]

I can't seem to find anywhere doing that. Darn unhelpful compiler diagnostics 😂

Going to see if Clang is any help

[DefaultRyan]

This is really weird. I saw that test\single_threaded_observable_vector.cpp was triggering this warning, so I tried to isolate an instance of it by commenting out parts of the file, until I isolated one. [Line 48]

(

cppwinrt/test/test/single_threaded_observable_vector.cpp

Line 48 in 37f0c4e

REQUIRE(sender == vector_i);

)

    vector_i.VectorChanged([&](IObservableVector<int> const& sender, IVectorChangedEventArgs const&)
        {
            changed_i = sender.GetAt(0) == 123;
            REQUIRE(sender == vector_i); // <--- THIS LINE TRIGGERS THE WARNING
        });

This is very mysterious to me. I'm almost beginning to suspect a compiler regression?

[sylveon]

I wonder if it could be catch testing for member begin but then using ADL begin

[sylveon]

I tried building single_threaded_observable_vector.cpp standalone in a brand new VS solution with warning level set to 4 and include path customized to point to my build of cppwinrt (also pointed to the vendored copy of catch), and the warning isn't present here...

[sylveon]

Found the issue!

cppwinrt/test/catch.hpp

Line 2006 in 122b732

struct is_range_impl<T, typename void_type<decltype(begin(std::declval<T>()))>::type> : std::true_type {

This silently started failing due to ambiguous overload errors (both std::begin and winrt::impl::begin are a match now), so the template expansion of std::begin was also expanding IVector<T>::begin, but it wasn't used because the ambiguous overload error was SFINAE'd out so is_range<IVector<T>> always returned false.

The issue can be reproduced like this:

void foo(winrt::Windows::Foundation::Collections::IVector<int> const& a)
{
    using std::begin;
	begin(a);
}

I haven't found a workaround yet unfortunately.

This probably needs some test coverage since using std::begin; is quite common for ADL.

[sylveon]

Found a solution: piggybacking off std::begin for our ADL begin, so in the end it resolves to the same function, which means it can't be ambiguous. This made catch's is_range work again, so it removed all the warnings as well.

Also added test coverage to avoid future breaks. I think this is ready for another internal build (and if everything's green, a merge) now.

[kennykerr]

Looks good to me - thanks!

[kennykerr]

2.0.210329.4 is the build that includes this feature.