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

iterator_traits<vehicle_part_iterator<T>> is missing reference #58643

Closed
StephanTLavavej opened this issue Jun 23, 2022 · 0 comments · Fixed by #58647
Closed

iterator_traits<vehicle_part_iterator<T>> is missing reference #58643

StephanTLavavej opened this issue Jun 23, 2022 · 0 comments · Fixed by #58647
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Good First Issue This is a good first issue for a new contributor (S2 - Confirmed) Bug that's been confirmed to exist

Comments

@StephanTLavavej
Copy link

Describe the bug

I work on MSVC's C++ Standard Library implementation, and we regularly build popular open-source projects, including yours, with our development toolsets in order to prevent regressions from breaking your code. This also allows us to provide advance notice of breaking changes - in this case, we've found an issue in your code that will cause compiler errors with an upcoming version of the Visual C++ toolset.

Specifically, we merged microsoft/STL#2447 implementing a new optimization in the minmax_element family of algorithms. This optimization involves inspecting iterator types to see if they're eligible for the optimization, and as part of that, we need to inspect iterator_traits<UserDefinedIterator>::reference at compile-time.

There's an iterator_traits<vehicle_part_iterator<T>> in your codebase that does not provide a reference typedef (or pointer):

template<class T> struct iterator_traits<vehicle_part_iterator<T>> {
using difference_type = size_t;
using value_type = vpart_reference;
// TODO: maybe change into random access iterator? This requires adding
// more operators to the iterator, which may not be efficient.
using iterator_category = std::forward_iterator_tag;
};

When compiled with the upcoming VS 2022 17.4 Preview 1 toolset or later, this will emit an error of the form:

error C2794: 'reference': is not a member of any direct or indirect base class of 'std::iterator_traits<_FwdIt>'
        with
        [
            _FwdIt=vehicle_part_iterator<vehicle_part_range>
        ]

The fix should be straightforward (and backwards-compatible with all previous toolsets, and other compilers). According to the C++ Working Paper N4910 25.3.2.3 [iterator.traits]/1, the reference and pointer typedefs should match what operator*() and operator->() return, respectively:

const vpart_reference &operator*() const {
cata_assert( vp_ );
return *vp_;
}
const vpart_reference *operator->() const {

Therefore, this should be added to iterator_traits<vehicle_part_iterator<T>>:

using reference = const vpart_reference &;
using pointer = const vpart_reference *;

Unrelated aside: I saw the comment "This requires adding more operators to the iterator, which may not be efficient." It is certainly tedious to provide the large number of operators required for random-access iterators (including the obscure n + iter). However, this will have a minimal impact on compile-time throughput (build speed), and will typically improve run-time performance. This is because many STL algorithms and data structures will detect random-access iterators automatically, and perform constant-time operations. If they only have forward-iterator operations to work with, they have to fall back to linear-time operations. A simple example is calculating the distance between two iterators (which is used by vector range construction and many other places). std::distance() will detect random-access iterators and call last - first which performs a single subtraction, but for forward iterators it must repeatedly increment a copy of first until it's equal to last.

Steps to reproduce

See bug description (requires VS 2022 17.4 Preview 1 or later to reproduce, which has not yet shipped).

Expected behavior

Cataclysm-DDA should compile without errors.

Screenshots

No response

Versions and configuration

Compile-time issue, not running the game. Again, requires VS 2022 17.4 Preview 1 (or the latest build of https://github.com/microsoft/STL ).

Additional context

No response

@StephanTLavavej StephanTLavavej added the (S1 - Need confirmation) Report waiting on confirmation of reproducibility label Jun 23, 2022
@BrettDong BrettDong added (S2 - Confirmed) Bug that's been confirmed to exist [C++] Changes (can be) made in C++. Previously named `Code` Good First Issue This is a good first issue for a new contributor and removed (S1 - Need confirmation) Report waiting on confirmation of reproducibility labels Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Good First Issue This is a good first issue for a new contributor (S2 - Confirmed) Bug that's been confirmed to exist
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants