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

Tue Aug 6 14:27:50 PDT 2024

================
@@ -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>);
----------------
cjdb wrote:

> > > > How frequently do we (libc++) need to update our existing verification tests?
> > > 
> > > 
> > > I'm not exactly sure, but there is probably something every 3-4 months on average.
> > 
> > 
> > Doesn't seem like it's all that frequent.
> 
> That's probably the case because we're currently relying on a fraction of the diagnostics clang produces. With every error and note we rely on the frequency will increase. A significant portion of the `.verify.cpp` tests are for deprecated symbols and `static_assert`s currently. Still, having to modify tests every few months seems like a high frequency to me compared to the thousands of tests that have simply existed for the better part of a decade. They tend to not be modified just because we update tools. In the rare cases we have to modify them, it's almost always a bug in the test or in the tool, neither of which are the culprit here.
> 
> > > I absolutely disagree here. "Just put it in sneakily" is _much_ worse than having imperfect code commited. I've seen before that you suggest stuff like `_LIBCPP_ALWAYS_INLINE` or `[[nodiscard]]`. I don't think it's OK for this kind of stuff to be added and then ask for forgiveness. This has significantly eroded my confidence that you give reasonable reviews, especially since people pushed back on these things before.
> > 
> > 
> > I would like to remind you that we have a community [Code of Conduct](https://llvm.org/docs/CodeOfConduct.html), and that it is not okay to accuse folks of being sneaky or unreasonable, just because there is disagreement on technical requests.
> 
> This isn't a disagreement about a technical question. We disagree on a technical question, but that's not what I'm complaining about here. The disagreement is in how you handle changes of this magnitude. This change isn't very visible to other contributors when doing it in a patch to implement a feature that we've essentially implemented 100 times already. We're in no way breaking new ground in this patch except for this change. This kind of change could easily be done in a patch to ask whether we want to do it, making it much more visible to others.

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.

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.

> 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.

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

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.

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