[PATCH] D38757: [libc++] Fix PR34898 - vector iterator constructors and assign method perform push_back instead of emplace_back.

[Eric Fiselier via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315994: [libc++] Fix PR34898 - vector iterator constructors 
and assign method perform… (authored by EricWF).

Changed prior to commit:
  https://reviews.llvm.org/D38757?vs=119301=119302#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38757

Files:
  libcxx/trunk/include/deque
  libcxx/trunk/include/list
  libcxx/trunk/include/vector
  
libcxx/trunk/test/std/containers/sequences/deque/deque.cons/assign_iter_iter.pass.cpp
  libcxx/trunk/test/std/containers/sequences/deque/deque.cons/iter_iter.pass.cpp
  
libcxx/trunk/test/std/containers/sequences/deque/deque.cons/iter_iter_alloc.pass.cpp
  
libcxx/trunk/test/std/containers/sequences/list/list.cons/input_iterator.pass.cpp
  
libcxx/trunk/test/std/containers/sequences/vector/vector.cons/assign_iter_iter.pass.cpp
  
libcxx/trunk/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
  
libcxx/trunk/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
  libcxx/trunk/test/support/container_test_types.h
  libcxx/trunk/test/support/emplace_constructible.h

Index: libcxx/trunk/include/deque
===
--- libcxx/trunk/include/deque
+++ libcxx/trunk/include/deque
@@ -1356,7 +1356,6 @@
 iterator insert(const_iterator __p, initializer_list __il)
 {return insert(__p, __il.begin(), __il.end());}
 #endif  // _LIBCPP_CXX03_LANG
-
 iterator insert(const_iterator __p, const value_type& __v);
 iterator insert(const_iterator __p, size_type __n, const value_type& __v);
 template 
@@ -2224,7 +2223,11 @@
!__is_forward_iterator<_InpIter>::value>::type*)
 {
 for (; __f != __l; ++__f)
+#ifdef _LIBCPP_CXX03_LANG
 push_back(*__f);
+#else
+emplace_back(*__f);
+#endif
 }
 
 template 
Index: libcxx/trunk/include/vector
===
--- libcxx/trunk/include/vector
+++ libcxx/trunk/include/vector
@@ -674,6 +674,17 @@
 const value_type* data() const _NOEXCEPT
 {return _VSTD::__to_raw_pointer(this->__begin_);}
 
+#ifdef _LIBCPP_CXX03_LANG
+_LIBCPP_INLINE_VISIBILITY
+void __emplace_back(const value_type& __x) { push_back(__x); }
+#else
+template 
+_LIBCPP_INLINE_VISIBILITY
+void __emplace_back(_Arg&& __arg) {
+  emplace_back(_VSTD::forward<_Arg>(__arg));
+}
+#endif
+
 _LIBCPP_INLINE_VISIBILITY void push_back(const_reference __x);
 
 #ifndef _LIBCPP_CXX03_LANG
@@ -1128,7 +1139,7 @@
 __get_db()->__insert_c(this);
 #endif
 for (; __first != __last; ++__first)
-push_back(*__first);
+__emplace_back(*__first);
 }
 
 template 
@@ -1145,7 +1156,7 @@
 __get_db()->__insert_c(this);
 #endif
 for (; __first != __last; ++__first)
-push_back(*__first);
+__emplace_back(*__first);
 }
 
 template 
@@ -1365,7 +1376,7 @@
 {
 clear();
 for (; __first != __last; ++__first)
-push_back(*__first);
+__emplace_back(*__first);
 }
 
 template 
Index: libcxx/trunk/include/list
===
--- libcxx/trunk/include/list
+++ libcxx/trunk/include/list
@@ -992,6 +992,15 @@
 void push_front(const value_type& __x);
 void push_back(const value_type& __x);
 
+#ifndef _LIBCPP_CXX03_LANG
+template 
+_LIBCPP_INLINE_VISIBILITY
+void __emplace_back(_Arg&& __arg) { emplace_back(_VSTD::forward<_Arg>(__arg)); }
+#else
+_LIBCPP_INLINE_VISIBILITY
+void __emplace_back(value_type const& __arg) { push_back(__arg); }
+#endif
+
 iterator insert(const_iterator __p, const value_type& __x);
 iterator insert(const_iterator __p, size_type __n, const value_type& __x);
 template 
@@ -1189,7 +1198,7 @@
 __get_db()->__insert_c(this);
 #endif
 for (; __f != __l; ++__f)
-push_back(*__f);
+__emplace_back(*__f);
 }
 
 template 
@@ -1202,7 +1211,7 @@
 __get_db()->__insert_c(this);
 #endif
 for (; __f != __l; ++__f)
-push_back(*__f);
+__emplace_back(*__f);
 }
 
 template 
Index: libcxx/trunk/test/support/emplace_constructible.h
===
--- libcxx/trunk/test/support/emplace_constructible.h
+++ libcxx/trunk/test/support/emplace_constructible.h
@@ -0,0 +1,74 @@
+#ifndef TEST_SUPPORT_EMPLACE_CONSTRUCTIBLE_H
+#define TEST_SUPPORT_EMPLACE_CONSTRUCTIBLE_H
+
+#include "test_macros.h"
+
+#if TEST_STD_VER >= 11
+template 
+struct EmplaceConstructible {
+  T value;
+  explicit EmplaceConstructible(T value) : value(value) {}
+  EmplaceConstructible(EmplaceConstructible const&) = delete;
+};
+
+template 
+struct EmplaceConstructibleAndMoveInsertable {
+  int copied = 0;
+  T value;
+  explicit EmplaceConstructibleAndMoveInsertable(T value) : value(value) {}
+
+  EmplaceConstructibleAndMoveInsertable(
+ 

[PATCH] D38757: [libc++] Fix PR34898 - vector iterator constructors and assign method perform push_back instead of emplace_back.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF updated this revision to Diff 119301.
EricWF added a comment.

- Update whitespace.


https://reviews.llvm.org/D38757

Files:
  include/deque
  include/list
  include/vector
  test/std/containers/sequences/deque/deque.cons/assign_iter_iter.pass.cpp
  test/std/containers/sequences/deque/deque.cons/iter_iter.pass.cpp
  test/std/containers/sequences/deque/deque.cons/iter_iter_alloc.pass.cpp
  test/std/containers/sequences/list/list.cons/input_iterator.pass.cpp
  test/std/containers/sequences/vector/vector.cons/assign_iter_iter.pass.cpp
  test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
  
test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
  test/support/container_test_types.h
  test/support/emplace_constructible.h

Index: test/support/emplace_constructible.h
===
--- /dev/null
+++ test/support/emplace_constructible.h
@@ -0,0 +1,74 @@
+#ifndef TEST_SUPPORT_EMPLACE_CONSTRUCTIBLE_H
+#define TEST_SUPPORT_EMPLACE_CONSTRUCTIBLE_H
+
+#include "test_macros.h"
+
+#if TEST_STD_VER >= 11
+template 
+struct EmplaceConstructible {
+  T value;
+  explicit EmplaceConstructible(T value) : value(value) {}
+  EmplaceConstructible(EmplaceConstructible const&) = delete;
+};
+
+template 
+struct EmplaceConstructibleAndMoveInsertable {
+  int copied = 0;
+  T value;
+  explicit EmplaceConstructibleAndMoveInsertable(T value) : value(value) {}
+
+  EmplaceConstructibleAndMoveInsertable(
+  EmplaceConstructibleAndMoveInsertable&& Other)
+  : copied(Other.copied + 1), value(std::move(Other.value)) {}
+};
+
+template 
+struct EmplaceConstructibleAndMoveable {
+  int copied = 0;
+  int assigned = 0;
+  T value;
+  explicit EmplaceConstructibleAndMoveable(T value) noexcept : value(value) {}
+
+  EmplaceConstructibleAndMoveable(EmplaceConstructibleAndMoveable&& Other)
+  noexcept : copied(Other.copied + 1),
+ value(std::move(Other.value)) {}
+
+  EmplaceConstructibleAndMoveable&
+  operator=(EmplaceConstructibleAndMoveable&& Other) noexcept {
+copied = Other.copied;
+assigned = Other.assigned + 1;
+value = std::move(Other.value);
+return *this;
+  }
+};
+
+template 
+struct EmplaceConstructibleMoveableAndAssignable {
+  int copied = 0;
+  int assigned = 0;
+  T value;
+  explicit EmplaceConstructibleMoveableAndAssignable(T value) noexcept
+  : value(value) {}
+
+  EmplaceConstructibleMoveableAndAssignable(
+  EmplaceConstructibleMoveableAndAssignable&& Other) noexcept
+  : copied(Other.copied + 1),
+value(std::move(Other.value)) {}
+
+  EmplaceConstructibleMoveableAndAssignable&
+  operator=(EmplaceConstructibleMoveableAndAssignable&& Other) noexcept {
+copied = Other.copied;
+assigned = Other.assigned + 1;
+value = std::move(Other.value);
+return *this;
+  }
+
+  EmplaceConstructibleMoveableAndAssignable& operator=(T xvalue) {
+value = std::move(xvalue);
+++assigned;
+return *this;
+  }
+};
+#endif
+
+#endif // TEST_SUPPORT_EMPLACE_CONSTRUCTIBLE_H
Index: test/support/container_test_types.h
===
--- test/support/container_test_types.h
+++ test/support/container_test_types.h
@@ -234,6 +234,19 @@
   return 
 }
 
+template 
+struct ExpectConstructGuard {
+  ExpectConstructGuard(int N)  {
+auto CC = getConstructController();
+assert(!CC->unchecked());
+CC->expect(N);
+  }
+
+  ~ExpectConstructGuard() {
+assert(!getConstructController()->unchecked());
+  }
+};
+
 //===--===//
 //   ContainerTestAllocator
 //===--===//
@@ -417,7 +430,12 @@
   return arg.data;
 }
   };
-
+  template 
+  class vector;
+  template 
+  class deque;
+  template 
+  class list;
   template 
   class map;
   template 
@@ -444,6 +462,13 @@
 // TCT - Test container type
 namespace TCT {
 
+template >
+using vector = std::vector >;
+template >
+using deque = std::deque >;
+template >
+using list = std::list >;
+
 template , class Value = CopyInsertable<2>,
   class ValueTp = std::pair >
 using unordered_map =
@@ -488,5 +513,4 @@
 
 } // end namespace TCT
 
-
 #endif // SUPPORT_CONTAINER_TEST_TYPES_H
Index: test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
===
--- test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
+++ test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
@@ -21,56 +21,152 @@
 #include "test_allocator.h"
 #include "min_allocator.h"
 #include "asan_testing.h"
+#if TEST_STD_VER >= 11
+#include "emplace_constructible.h"
+#include 

[PATCH] D38757: [libc++] Fix PR34898 - vector iterator constructors and assign method perform push_back instead of emplace_back.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF accepted this revision.
EricWF added a comment.

In https://reviews.llvm.org/D38757#897536, @dlj wrote:

> Hmm, looking more at this change... while it does make the behaviour 
> consistent for Forward and Input iterators, I think it's just making them 
> both do the wrong thing.
>
> Specifically, based on this:
>
> "... i and j denote iterators satisfying input iterator requirements and 
> refer to elements implicitly convertible to value_­type..."
>
> https://timsong-cpp.github.io/cppwp/n4659/container.requirements#sequence.reqmts-3
>
> So, for example, in test_emplacable_concept, the vector constructor should be 
> diagnosed, because there is no way to *implicitly* convert from the 
> dereferenced iterator type to the inserted type. The selected constructor is 
> explicit. Using emplacement just omits a *second* potentially-expensive 
> conversion: the explicit constructor behaviour (invoked through forwarding) 
> may still be undesired.


No, these types should be EmplaceConstructed if construction is supposed to 
occur when we are constructing a new element in the container, which is what 
this test does. These elements *need* to be constructed using the Allocator. 
The implicit construction is useful in PR34906 
, which we implicitly construct a 
value and then assign it to an already existing container.

This patch is correct to forward to emplace instead of push_back.




Comment at: 
test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp:55
 int a[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 8, 7, 6, 5, 4, 3, 1, 0};
-int* an = a + sizeof(a)/sizeof(a[0]);
+int* an = a + sizeof(a) / sizeof(a[0]);
 std::allocator alloc;

Sorry, this is all just re-formatting.



Comment at: test/support/emplace_constructible.h:6
+
+#if TEST_STD_VER >= 11
+  template 

THis file needs re-formatting.


https://reviews.llvm.org/D38757



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38757: [libc++] Fix PR34898 - vector iterator constructors and assign method perform push_back instead of emplace_back.

[David L. Jones via Phabricator via cfe-commits]

dlj requested changes to this revision.
dlj added a comment.
This revision now requires changes to proceed.

Hmm, looking more at this change... while it does make the behaviour consistent 
for Forward and Input iterators, I think it's just making them both do the 
wrong thing.

Specifically, based on this:

"... i and j denote iterators satisfying input iterator requirements and refer 
to elements implicitly convertible to value_­type..."

https://timsong-cpp.github.io/cppwp/n4659/container.requirements#sequence.reqmts-3

So, for example, in test_emplacable_concept, the vector constructor should be 
diagnosed, because there is no way to *implicitly* convert from the 
dereferenced iterator type to the inserted type. The selected constructor is 
explicit. Using emplacement just omits a *second* potentially-expensive 
conversion: the explicit constructor behaviour (invoked through forwarding) may 
still be undesired.


https://reviews.llvm.org/D38757



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38757: [libc++] Fix PR34898 - vector iterator constructors and assign method perform push_back instead of emplace_back.

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.
This revision is now accepted and ready to land.

LGTM - thanks!


https://reviews.llvm.org/D38757



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits