Skip to content

Commit

Permalink
Increased std::vector compatibility of fbvector
Browse files Browse the repository at this point in the history
Summary:
fbvector was not accepting move_iterators for
InputIterator-templated construct, assign, and insert. The root cause
was the check '(b_ <= &*first && &*first < e_)', used to check if the
assignment was from internal data. This addressof check does not compile
with rvalue-references, and causes the first iterator to be dereferenced
more than once; both against the contract of std::vector. The standard
allows for undefined behaviour in the self-iterator case, so there are
no contractual barriers to removing this check.

Test Plan:
run fbvector test and benchmark. An fbgs for 'assign' turns
up few matches; the seem to be normal use-case. Test fbvector assign
with self-iterators in order (i.e. fbv.assign(fbv.begin(), fbv.end()));
this seems to work fine.

Reviewed By: andrei.alexandrescu@fb.com

FB internal diff: D535012
  • Loading branch information
Gownta authored and tudor committed Aug 26, 2012
1 parent cc1518d commit bf188b3
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 12 deletions.
12 changes: 0 additions & 12 deletions folly/FBVector.h
Expand Up @@ -376,13 +376,6 @@ class fbvector : private boost::totally_ordered<fbvector<T,Allocator> > {
void assignImpl(InputIterator first, InputIterator last, boost::false_type) {
// Pair of iterators
if (fbvector_detail::isForwardIterator<InputIterator>::value) {
if (b_ <= &*first && &*first < e_) {
// Aliased assign, work on the side
fbvector result(first, last);
result.swap(*this);
return;
}

auto const oldSize = size();
auto const newSize = std::distance(first, last);

Expand Down Expand Up @@ -802,10 +795,6 @@ class fbvector : private boost::totally_ordered<fbvector<T,Allocator> > {
// Can compute distance
auto const n = std::distance(first, last);
if (e_ + n >= z_) {
if (b_ <= &*first && &*first < e_) {
// Ew, aliased insert
goto conservative;
}
auto const m = position - b_;
reserve(size() + n);
position = b_ + m;
Expand All @@ -828,7 +817,6 @@ class fbvector : private boost::totally_ordered<fbvector<T,Allocator> > {
} else {
// Cannot compute distance, crappy approach
// TODO: OPTIMIZE
conservative:
fbvector result(cbegin(), position);
auto const offset = result.size();
FOR_EACH_RANGE (i, first, last) {
Expand Down
22 changes: 22 additions & 0 deletions folly/test/FBVectorTest.cpp
Expand Up @@ -223,6 +223,28 @@ TEST(FBVector, task858056) {
EXPECT_EQ("Cycle detected: [baz] [bar] [foo] ", message);
}

TEST(FBVector, move_iterator) {
fbvector<int> base = { 0, 1, 2 };

auto cp1 = base;
fbvector<int> fbvi1(std::make_move_iterator(cp1.begin()),
std::make_move_iterator(cp1.end()));
EXPECT_EQ(fbvi1, base);

auto cp2 = base;
fbvector<int> fbvi2;
fbvi2.assign(std::make_move_iterator(cp2.begin()),
std::make_move_iterator(cp2.end()));
EXPECT_EQ(fbvi2, base);

auto cp3 = base;
fbvector<int> fbvi3;
fbvi3.insert(fbvi3.end(),
std::make_move_iterator(cp3.begin()),
std::make_move_iterator(cp3.end()));
EXPECT_EQ(fbvi3, base);
}

int main(int argc, char** argv) {
testing::InitGoogleTest(&argc, argv);
google::ParseCommandLineFlags(&argc, &argv, true);
Expand Down

0 comments on commit bf188b3

Please sign in to comment.