microsoft / GSL

Guidelines Support Library
Other
6.13k stars 737 forks source link

Fix `span_iterator` unwrapping for MSVC #1100

Closed StephanTLavavej closed 1 year ago

StephanTLavavej commented 1 year ago

@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 gsl::details::span_iterator::operator -(const gsl::details::span_iterator::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::difference_type gsl::details::span_iterator::operator -(const gsl::details::span_iterator &) 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::difference_type gsl::details::span_iterator::operator -(const gsl::details::span_iterator &) noexcept const': could not deduce template argument for 'const gsl::details::span_iterator &' 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()' 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()': could not deduce template argument for 'const std::move_iterator<_Iter> &' from '_Sent' with [ _Sent=gsl::details::span_iterator ] 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()' 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()': could not deduce template argument for 'const std::reverse_iterator<_BidIt> &' from '_Sent' with [ _Sent=gsl::details::span_iterator ] 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 ] meow.cpp(8): note: see reference to function template instantiation 'gsl::details::span_iterator std::ranges::_Sort_fn::operator ()&,std::ranges::less,std::identity>(_Rng,_Pr,_Pj) const' being compiled with [ ElementType=int, _Rng=gsl::span &, _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 commented 1 year ago

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

dmitrykobets-msft commented 1 year ago

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