llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.13k stars 12.02k forks source link

[libc++] `vector::insert_range` should not use `std::copy` #115801

Open hewillk opened 5 days ago

hewillk commented 5 days ago

https://github.com/llvm/llvm-project/blob/de2fad32513f7420988df1cf99aff90e0a067469/libcxx/include/__vector/vector.h#L1317

Unlike assign_range which requires assignable_from<T&, ranges​::​range_reference_t<R>> , this means that std::copy is not necessarily well-formed for insert_range:

#include <vector>

struct Int {
  void operator=(int) = delete;
  Int(int);
};

int main() {
  std::vector<Int> v;
  v.insert_range(v.begin(), std::vector{42});
}

https://godbolt.org/z/jo3vjjo5b

Additionally, range-version APIs should not dispatch to the std::moew algorithm which only works with C++98 iterators.

frederick-vs-ja commented 5 days ago

Additionally, range-version APIs should not dispatch to the std::moew algorithm which only works with C++98 iterators.

This shouldn't be an issue after implementing P2408R5 (tracked in #105211). I think we should implement P2408R5 in C++20 mode like MSVC STL.

hewillk commented 5 days ago

Additionally, range-version APIs should not dispatch to the std::moew algorithm which only works with C++98 iterators.

This shouldn't be an issue after implementing P2408R5 (tracked in #105211). I think we should implement P2408R5 in C++20 mode like MSVC STL.

Why is it not an issue? As far as I know, that paper is mainly about forward_iterator not input_iterator.

frederick-vs-ja commented 5 days ago

Additionally, range-version APIs should not dispatch to the std::moew algorithm which only works with C++98 iterators.

This shouldn't be an issue after implementing P2408R5 (tracked in #105211). I think we should implement P2408R5 in C++20 mode like MSVC STL.

Why is it not an issue? As far as I know, that paper is mainly about forward_iterator not input_iterator.

I see. Currently, __insert_with_size doesn't properly work with input-only ranges because it possibly traverses a subrange more than once. We need to carve out the input-only-but-sized cases first, and the rest will be resolved together with P2408R5.

frederick-vs-ja commented 5 days ago

and the rest will be resolved together with P2408R5

Not true. I must had forgotten something. Per [sequence.reqmts]/36, IIUC both the old iterator-pair insert and the new insert_range are not suppose to directly assign any container element from the source range, except when the assignment happens to be move assignment of the value_type.

I.e. the old iterator-pair insert is almost equivalently misimplemented.

philnik777 commented 5 days ago

This seems like a duplicate of #47963.