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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
