[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 <https://bugs.llvm.org/show_bug.cgi?id=34906>, 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<int> alloc;
----------------
Sorry, this is all just re-formatting.


================
Comment at: test/support/emplace_constructible.h:6
+
+#if TEST_STD_VER >= 11
+  template <class T>
----------------
THis file needs re-formatting.


https://reviews.llvm.org/D38757