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

VS 2022 17.4 reproduces different result with std::ranges compared to older version #3199

Closed
MennoK opened this issue Nov 9, 2022 · 4 comments
Labels
invalid This issue is incorrect or by design ranges C++20/23 ranges

Comments

@MennoK
Copy link

MennoK commented Nov 9, 2022

Copying a view with filter and transform in VS 2022 17.4 (exact compiler version is MSVC 19.34.31933.0) to a vector behaves differently compared to older versions.

Below is a small code snippet is shown. The code should filter unique integer values and transform them to a string, and should be copied to a new vector.

#include <iostream>
#include <ranges>
#include <string>
#include <unordered_set>
#include <vector>

int main() {
    const std::vector<int> v = {1, 2, 2};
    std::unordered_set<int> set{};
    auto view =
        v | std::views::filter([&set](const int i) {
            std::cout << "checking filter for " << i << '\n';
            auto it = set.find(i);
            if (it != set.end()) return false;
            set.insert(i);
            return true;
        }) |
        std::views::transform([](const int i) { return std::to_string(i); });
    const auto js = std::vector<std::string>(view.begin(), view.end());
    std::cout << "js size is " << js.size() << '\n';
}

Godbolt link: https://godbolt.org/z/d7TTfq9K5

The expected result is that std::vector<std::string> js is size 2 and only includes "1" and "2", but with the newer VS compiler the vector is size 1 and contains only "1",

I'm not 100% sure whether this code sample has always been wrong, and the older compilers were printing the correct output by accident. Or a bug has been introduced with VS 2022 17.4..

@fsb4000
Copy link
Contributor

fsb4000 commented Nov 9, 2022

It looks like a STL bug.

Thanks for reporting!

@JMazurkiewicz
Copy link
Contributor

JMazurkiewicz commented Nov 9, 2022

This is not a bug. Short explanation:

What was happening before VS 17.4

Iterators of your view were treated as Cpp17InputIterators by vector's constructor, so the range was traversed exactly once. That's why the output of your program is:

# The filter's predicate is checked exactly once for each element (one traversal)
checking filter for 1
checking filter for 2
checking filter for 2
js size is 2 # Only first two values got inserted

Vector constructor in 17.3:

STL/stl/inc/vector

Lines 693 to 715 in 60decd0

template <class _Iter, enable_if_t<_Is_iterator_v<_Iter>, int> = 0>
_CONSTEXPR20 vector(_Iter _First, _Iter _Last, const _Alloc& _Al = _Alloc())
: _Mypair(_One_then_variadic_args_t{}, _Al) {
_Adl_verify_range(_First, _Last);
auto _UFirst = _Get_unwrapped(_First);
auto _ULast = _Get_unwrapped(_Last);
if constexpr (_Is_fwd_iter_v<_Iter>) {
const auto _Count = _Convert_size<size_type>(static_cast<size_t>(_STD distance(_UFirst, _ULast)));
_Construct_n(_Count, _STD move(_UFirst), _STD move(_ULast));
} else {
auto&& _Alproxy = _GET_PROXY_ALLOCATOR(_Alty, _Getal());
_Container_proxy_ptr<_Alty> _Proxy(_Alproxy, _Mypair._Myval2);
_Tidy_guard<vector> _Guard{this};
for (; _UFirst != _ULast; ++_UFirst) {
// performance note: _Emplace_one_at_back's strong guarantee is unnecessary here.
_Emplace_one_at_back(*_UFirst);
}
_Guard._Target = nullptr;
_Proxy._Release();
}
}

What is going on in VS 17.4

#1794 changed vector's iterator constructor and now your view's iterators are handled as std::forward_iterators, so the range is being traversed twice - for the first time to find the size of the range and for the second time to construct the vector.

Output of the program from your "Compiler Explorer" example:

# FIRST TRAVERSAL (to find the size)
checking filter for 1
checking filter for 2
checking filter for 2
# SECOND TRAVERSAL (to construct the vector)
# The `begin` iterator is cached by filter_view at this point, so the predicate is not checked for `1`.
checking filter for 2
checking filter for 2
js size is 1 # Only "1" got inserted, because of caching

General problem

The general problem is that this program has undefined behaviour - the predicate of your filter does not model std::indirect_unary_predicate concept.

@StephanTLavavej StephanTLavavej added invalid This issue is incorrect or by design ranges C++20/23 ranges labels Nov 9, 2022
@StephanTLavavej
Copy link
Member

Thanks @JMazurkiewicz - we talked about this at the weekly maintainer meeting and your analysis is perfect. 😻 The behavior is indeed undefined because the predicate returns different answers when repeatedly invoked.

Closing this issue as by design - thanks for the report.

@StephanTLavavej StephanTLavavej closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2022
@MennoK
Copy link
Author

MennoK commented Nov 10, 2022

Thank you for your extensive answer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This issue is incorrect or by design ranges C++20/23 ranges
Projects
None yet
Development

No branches or pull requests

4 participants