This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] Implement the changes to vector from P1206 (`ranges::to`):
ClosedPublic

Authored by var-const on May 4 2023, 2:13 AM.

Details

Summary
  • add the from_range_t constructors and the related deduction guides;
  • add the insert_range/assign_range/etc. member functions.

(Note: this patch is split from https://reviews.llvm.org/D142335)

Diff Detail

Event Timeline

var-const created this revision.May 4 2023, 2:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2023, 2:13 AM
var-const requested review of this revision.May 4 2023, 2:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2023, 2:13 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.May 4 2023, 9:42 AM
ldionne added a subscriber: ldionne.

LGTM with passing CI! Thanks!

libcxx/include/__ranges/from_range.h
24

You should also update the synopsis in <ranges> with this.

28

I think you forgot to copy over the tests for libcxx/test/std/ranges/range.utility/range.utility.conv/from_range_t.compile.pass.cpp.

libcxx/include/vector
1484

We still need a test to catch the issue that went uncaught in D142335.

2874

It feels like passing __n might be more appropriate, given that __construct_at_end takes size_type.

2889

Same.

2905

Here and below, I think it is correct as-is since you're using __v.size().

This revision is now accepted and ready to land.May 4 2023, 9:42 AM
var-const updated this revision to Diff 519692.May 4 2023, 4:39 PM
var-const marked 5 inline comments as done.

Address feedback

libcxx/include/vector
1484

Added a test case to test/std/containers/sequences/vector/vector.modifiers/assign_range.pass.cpp. The case is that the range being assigned is longer than the vector but within the vector's capacity.

var-const updated this revision to Diff 519693.May 4 2023, 4:40 PM

Generated files.

var-const updated this revision to Diff 519696.May 4 2023, 4:52 PM

Copy over the changes to build files.

var-const updated this revision to Diff 520242.May 7 2023, 8:43 PM

Clang-format the newly-added files.

var-const updated this revision to Diff 520287.May 8 2023, 12:35 AM
  • try the ignore_format.txt generated by the CI;
  • don't run Asan checks on vector<bool>, it's not supported;
  • rebase.
var-const updated this revision to Diff 520506.May 8 2023, 4:10 PM

Work around a GCC bug and a warning.

var-const updated this revision to Diff 520557.May 8 2023, 7:36 PM

Appease the CI

var-const added subscribers: Restricted Project, rupprecht.May 8 2023, 11:42 PM

@rupprecht Just a heads-up: this patch refactors the internals of vector and thus carries some amount of risk of a possible breakage. The tests in this patch are quite extensive, but I'd still like to give you a warning just in case you notice any new failures that this patch could be the culprit.

Adding libc++-vendors to subscribers as well.

asmok-g added a subscriber: asmok-g.EditedMay 12 2023, 10:40 AM

Before this patch, if a non-copy-constructible Iterator was passed to the vector constructor vector<_Tp, _Allocator>::vector(_InputIterator __first, _InputIterator __last) it compiled, but now it gives the error: call to deleted constructor of 'Iterator'. Is that expected ?

smth like

#include <iterator>
#include <vector>

const char* data = "abc";

class Iterator : public std::iterator<std::input_iterator_tag, const char> {
 public:
  explicit Iterator(const char c) {}
  Iterator(const Iterator& other) = delete;
  Iterator& operator=(const Iterator& other) = delete;
  reference operator*() const { return data[0]; }
  void operator++() {}
  bool operator==(const Iterator& other) const { return true; }
  bool operator!=(const Iterator& other) const { return false; }
};

class Elem {
 public:
  Elem() = default;
  explicit Elem(const char ref) {}
};

int main(int argc, char** argv) {
  std::vector<Elem>(Iterator(data[0]), Iterator(data[1]));
}

@rupprecht Just a heads-up: this patch refactors the internals of vector and thus carries some amount of risk of a possible breakage. The tests in this patch are quite extensive, but I'd still like to give you a warning just in case you notice any new failures that this patch could be the culprit.

Adding libc++-vendors to subscribers as well.

Before this patch, if a non-copy-constructible Iterator was passed to the vector constructor vector<_Tp, _Allocator>::vector(_InputIterator __first, _InputIterator __last) it compiled, but now it gives the error: call to deleted constructor of 'Iterator'. Is that expected ?

smth like

#include <iterator>
#include <vector>

const char* data = "abc";

class Iterator : public std::iterator<std::input_iterator_tag, const char> {
 public:
  explicit Iterator(const char c) {}
  Iterator(const Iterator& other) = delete;
  Iterator& operator=(const Iterator& other) = delete;
  reference operator*() const { return data[0]; }
  void operator++() {}
  bool operator==(const Iterator& other) const { return true; }
  bool operator!=(const Iterator& other) const { return false; }
};

class Elem {
 public:
  Elem() = default;
  explicit Elem(const char ref) {}
};

int main(int argc, char** argv) {
  std::vector<Elem>(Iterator(data[0]), Iterator(data[1]));
}

Thank you for the report! Is this blocking you?

I will look into this today. Hopefully it's an easy fix.

@rupprecht Just a heads-up: this patch refactors the internals of vector and thus carries some amount of risk of a possible breakage. The tests in this patch are quite extensive, but I'd still like to give you a warning just in case you notice any new failures that this patch could be the culprit.

Adding libc++-vendors to subscribers as well.

Before this patch, if a non-copy-constructible Iterator was passed to the vector constructor vector<_Tp, _Allocator>::vector(_InputIterator __first, _InputIterator __last) it compiled, but now it gives the error: call to deleted constructor of 'Iterator'. Is that expected ?

smth like

#include <iterator>
#include <vector>

const char* data = "abc";

class Iterator : public std::iterator<std::input_iterator_tag, const char> {
 public:
  explicit Iterator(const char c) {}
  Iterator(const Iterator& other) = delete;
  Iterator& operator=(const Iterator& other) = delete;
  reference operator*() const { return data[0]; }
  void operator++() {}
  bool operator==(const Iterator& other) const { return true; }
  bool operator!=(const Iterator& other) const { return false; }
};

class Elem {
 public:
  Elem() = default;
  explicit Elem(const char ref) {}
};

int main(int argc, char** argv) {
  std::vector<Elem>(Iterator(data[0]), Iterator(data[1]));
}

Thank you for the report! Is this blocking you?

I will look into this today. Hopefully it's an easy fix.

An InputIterator has to be copy-constructible: http://eel.is/c++draft/iterator.iterators#2.1, and IMO we should enforce that even if we don't make use of the functionality (e.g. static_assert(is_copy_constructible<_InputIterator>::value, "InputIterator has to be copy constructible")).

Thanks for quick response!

Thank you for the report! Is this blocking you?

yes. it's causing breakages in google code.

I will look into this today. Hopefully it's an easy fix.

Great!

I must say I agree with @philnik here. Iterators are supposed to be copyable, and implementations are free to use that in their implementation as a detail. For example, here we're just passing the iterators down to another function. We can see that GCC also does not accept this code: https://godbolt.org/z/7GhGf8Exo

I don't think it would be reasonable to provide a ad-hoc guarantee like that in std::vector's constructor. In fact, I think our original sin was probably not to add a static_assert in some of these methods so we started accepting types that we shouldn't have accepted, and then Hyrum's law materialized.

@asmok-g So in the end, the code will have to be fixed (and the fix is pretty trivial). However, our goal is not to make your life hard. If this breakage is very wide and challenging for you folks to fix, we can try to add a workaround on our side and give you a grace period. We're willing to work with you to make this as easy as possible, but the right thing to do in the end is to fix the code, not silently accept it.

Please let us know what the scope of breakage is and whether you want us to work on providing you a grace period.