This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by EricWF on Oct 10 2017, 12:39 PM.

Details

Summary

The constructors vector(Iter, Iter, Alloc = Alloc{}) and assign(Iter, Iter) don't correctly perform EmplaceConstruction from the result of dereferencing the iterator. This results in them performing an additional and unneeded copy.

This patch addresses the issue by correctly using emplace_back in C++11 and newer.

There are also some bugs in our insert implementation, but those will be handled separately.

@mclow.lists We should probably merge this into 5.1, agreed?

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF created this revision.Oct 10 2017, 12:39 PM
EricWF updated this revision to Diff 118474.Oct 10 2017, 2:02 PM
  • Add additional fixes for vector, deque, and list.
mclow.lists accepted this revision.Oct 11 2017, 8:34 AM

LGTM - thanks!

This revision is now accepted and ready to land.Oct 11 2017, 8:34 AM
dlj accepted this revision.Oct 12 2017, 9:49 AM
dlj requested changes to this revision.Oct 13 2017, 5:06 PM

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.

This revision now requires changes to proceed.Oct 13 2017, 5:06 PM
EricWF accepted this revision.Oct 17 2017, 5:31 AM
In 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.

test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
55 ↗(On Diff #118455)

Sorry, this is all just re-formatting.

test/support/emplace_constructible.h
6 ↗(On Diff #118474)

THis file needs re-formatting.

EricWF updated this revision to Diff 119301.Oct 17 2017, 6:01 AM
EricWF edited edge metadata.
  • Update whitespace.
This revision was automatically updated to reflect the committed changes.