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

Fix span_iterator unwrapping for MSVC #1100

Merged

Conversation

StephanTLavavej
Copy link
Member

@tiagomacarios encountered the following error:

C:\Temp>type meow.cpp
#include <algorithm>
#include <gsl/span>
#include <iostream>

int main() {
    int arr[]{1, 7, 2, 9};
    gsl::span sp{arr, std::size(arr)};
    std::ranges::sort(sp);
    for (const auto& e : sp) {
        std::cout << e << ", ";
    }
    std::cout << "\n";
}
C:\Temp>cl /EHsc /nologo /W4 /MTd /Od /std:c++latest /I GSL\include meow.cpp && meow
meow.cpp
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.36.32323\include\algorithm(8268): error C2678: binary '-': no operator found which takes a left-hand operand of type '_Sent' (or there is no acceptable conversion)
        with
        [
            _Sent=gsl::details::span_iterator<int>
        ]
Click to expand error context (which explains the proximate error, but in a verbose, complicated way):
GSL\include\gsl/span(221): note: could be 'gsl::details::span_iterator<ElementType> gsl::details::span_iterator<ElementType>::operator -(const gsl::details::span_iterator<ElementType>::difference_type) noexcept const'
        with
        [
            ElementType=int
        ]
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.36.32323\include\algorithm(8268): note: or       'built-in C++ operator-(int *, int *)'
GSL\include\gsl/span(231): note: or       'gsl::details::span_iterator<ElementType>::difference_type gsl::details::span_iterator<ElementType>::operator -(const gsl::details::span_iterator<Type2> &) noexcept const'
        with
        [
            ElementType=int
        ]
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.36.32323\include\algorithm(8268): note: 'gsl::details::span_iterator<ElementType>::difference_type gsl::details::span_iterator<ElementType>::operator -(const gsl::details::span_iterator<Type2> &) noexcept const': could not deduce template argument for 'const gsl::details::span_iterator<Type2> &' from 'int *'
        with
        [
            ElementType=int
        ]
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.36.32323\include\xutility(4216): note: or       'unknown-type std::operator -(const std::move_iterator<_Iter> &,const std::move_iterator<_Iter2> &) noexcept(<expr>)'
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.36.32323\include\algorithm(8268): note: 'unknown-type std::operator -(const std::move_iterator<_Iter> &,const std::move_iterator<_Iter2> &) noexcept(<expr>)': could not deduce template argument for 'const std::move_iterator<_Iter> &' from '_Sent'
        with
        [
            _Sent=gsl::details::span_iterator<int>
        ]
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.36.32323\include\xutility(1571): note: or       'unknown-type std::operator -(const std::reverse_iterator<_BidIt> &,const std::reverse_iterator<_BidIt2> &) noexcept(<expr>)'
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.36.32323\include\algorithm(8268): note: 'unknown-type std::operator -(const std::reverse_iterator<_BidIt> &,const std::reverse_iterator<_BidIt2> &) noexcept(<expr>)': could not deduce template argument for 'const std::reverse_iterator<_BidIt> &' from '_Sent'
        with
        [
            _Sent=gsl::details::span_iterator<int>
        ]
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.36.32323\include\algorithm(8268): note: while trying to match the argument list '(_Sent, int *)'
        with
        [
            _Sent=gsl::details::span_iterator<int>
        ]
meow.cpp(8): note: see reference to function template instantiation 'gsl::details::span_iterator<ElementType> std::ranges::_Sort_fn::operator ()<gsl::span<ElementType,18446744073709551615>&,std::ranges::less,std::identity>(_Rng,_Pr,_Pj) const' being compiled
        with
        [
            ElementType=int,
            _Rng=gsl::span<int,18446744073709551615> &,
            _Pr=std::ranges::less,
            _Pj=std::identity
        ]
[...]

The root cause is that GSL's span_iterator partially participates in MSVC's (unfortunately undocumented) iterator unwrapping machinery. The ranges algorithms require more complicated unwrapping tech than the classic algorithms, since ranges algorithms can mix iterators with sentinels.

The fix is to add a _Prevent_inheriting_unwrap typedef to span_iterator. As the name tersely indicates, this typedef allows the unwrapping machinery to have confidence that it's using unwrapping member functions that have been provided by the class itself and not any base class (because if this were inherited by a DerivedSpanIterator, the typedef would still name the base span_iterator). This single typedef activates all of the necessary ranges unwrapping machinery (a long, complicated tale) and allows the code to compile. With this fix:

C:\Temp>pushd GSL && git switch prevent-inheriting-unwrap && popd
Switched to branch 'prevent-inheriting-unwrap'

C:\Temp>cl /EHsc /nologo /W4 /MTd /Od /std:c++latest /I GSL\include meow.cpp && meow
meow.cpp
1, 2, 7, 9,

This is the exact same thing that STL iterators do, e.g. see https://github.com/microsoft/STL/blob/daa994bfc41c36196c536f2b68388f859d6bd656/stl/inc/vector#L211 for std::_Vector_const_iterator.

@beinhaerter
Copy link
Contributor

Wouldn't it be useful to also add a unit test for the change?

@dmitrykobets-msft
Copy link
Member

Thanks! I'll add a test for this in a followup PR

@dmitrykobets-msft dmitrykobets-msft merged commit 50d6eef into microsoft:main Mar 14, 2023
@StephanTLavavej StephanTLavavej deleted the prevent-inheriting-unwrap branch March 14, 2023 23:26
dmitrykobets-msft added a commit that referenced this pull request Mar 30, 2023
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

Successfully merging this pull request may close these issues.

None yet

5 participants