[libcxx-commits] [libcxx] [libc++] Implement ranges::iota (PR #68494)

[Nikolas Klauser via libcxx-commits]

================
@@ -0,0 +1,215 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Testing std::ranges::iota
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+#include <algorithm>
+#include <array>
+#include <cassert>
+#include <numeric>
+#include <utility>
+
+#include "almost_satisfies_types.h"
+#include "test_iterators.h"
+#include "test_macros.h"
+
+//
+// Testing constraints
+//
+
+// Concepts to check different overloads of std::ranges::iota
+template <class Iter = int*, class Sent = int*, class Value = int>
+concept HasIotaIter = requires(Iter&& iter, Sent&& sent, Value&& val) {
+  std::ranges::iota(std::forward<Iter>(iter), std::forward<Sent>(sent), std::forward<Value>(val));
+};
+
+template <class Range, class Value = int>
+concept HasIotaRange =
+    requires(Range&& range, Value&& val) { std::ranges::iota(std::forward<Range>(range), std::forward<Value>(val)); };
+
+// Test constraints of the iterator/sentinel overload
+// ==================================================
+static_assert(HasIotaIter<int*, int*, int>);
+
+// !input_or_output_iterator<O>
+static_assert(!HasIotaIter<InputIteratorNotInputOrOutputIterator>);
+
+// !sentinel_for<S, O>
+static_assert(!HasIotaIter<int*, SentinelForNotSemiregular>);
+static_assert(!HasIotaIter<int*, SentinelForNotWeaklyEqualityComparableWith>);
+
+// !weakly_incrementable<T>
+static_assert(!HasIotaIter<int*, int*, WeaklyIncrementableNotMovable>);
+
+// !indirectly writable <O, T>
+static_assert(!HasIotaIter<OutputIteratorNotIndirectlyWritable, int*, int>);
+
+// Test constraints for the range overload
+// =======================================
+static_assert(HasIotaRange<UncheckedRange<int*>, int>);
+
+// !weakly_incrementable<T>
+static_assert(!HasIotaRange<UncheckedRange<int*>, WeaklyIncrementableNotMovable>);
+
+// !ranges::output_range<const _Tp&>
+static_assert(!HasIotaRange<UncheckedRange<int*>, OutputIteratorNotIndirectlyWritable>);
----------------
philnik777 wrote:

> This request is for long-term maintainability, and confidence in the code. I'm not asking for drastic changes to be made here: I'm asking for a single test to either be rewritten in a way that captures the missing functionality, or to add a new test that captures it if the existing test is fulfilling a role that the rewrite does not capture. Just because we have done something one way a hundred times is not a good argument for us to continue doing it that way with new code. Software engineering is based on incremental change, and that is what I am asking for.

Because we did it a hundred times, it's a good idea to not change it in the 101st iteration, but instead separately. I don't think it's a simple extension of the test and I think I've made that pretty clear.

> As for visibility, I'm not sure what you're referring to: I have publicly requested the change in a public forum. That is rather visible.

People tend to not look through every PR they see. They simply don't have the time for that. If they see the 100th time that a ranges algorithm is implemented, they don't expect a significant change in how it's done. What I'm asking for is a separate PR which clearly states in the title what change you're proposing here. That tends to spark people's interest, since it's not the 100th time they see it.

> > I'm also not intending to accuse you of being unreasonable. 99% of the work you do is great, but there are a few changes you requested which were very much out-of-place on the review. For that reason I feel I have to check that you're not suggesting something novel to people who don't know the code base very well.
> > > With that in mind, it is important to note that incremental changes are a software engineering practice broadly adopted across the industry. This is a practice that LLVM routinely adopts, and it is entirely reasonable to request such changes in a code review. It's fine to disagree with technical suggestions: we can discuss those disagreements and work out what the best path forward is for the whole community.
> > 
> > 
> > I agree that it's fine to disagree on technical questions. To me it sounds like you don't want to talk to the whole community though. That's the disagreement I have.
> 
> At what point have I expressed a desire to avoid discussion about this? Just a few comments up, I said that we should have a discussion about the material, using the changes from this PR as information for future work.

That discussion should be done _before_ the change, not after. That's what I meant with "asking for forgiveness later".

> If you aren't complaining about something technical, then why is it being discussed in a pull request? More appropriate avenues of discussion for such a topic include:
> 
>     * directly messaging me to understand what I'm doing and why, so that we can collaborate on a path forward
> 
>     * asking in the libc++ group chat if you want a more visible discussion
> 
>     * escalating to the Code of Conduct committee, if you believe my behaviour is a violation of the rules put in place

If you think that's more productive, feel free to send me a PR so we can set up a meeting to discuss this further.
 
> It feels like a lot of time has been spent telling me what my intentions are; no time has been spent asking questions to confirm that these suspicions are grounded, or even to gather information on understanding why there are conflicting technical viewpoints, both of which are critical for collaboration. I would appreciate it if more time was spent doing the latter before concluding that a person is behaving in an antagonistic fashion towards the libc++ community.

I can't find a single place where I told you what your intentions are. I simply stated how I perceived some of your actions.


https://github.com/llvm/llvm-project/pull/68494