[libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

[EricWF]

Summary

In $:\Dev\msvc\src\qa\VC\Libs\libcxx\upstream\test\std\containers\map_allocator_requirement_test_templates.h and $\test\std\containers\set_allocator_requirement_test_templates.h, libcxx tests are asserting that the container's insert operation passes an incorrect type down to allocator::construct. Specifically, given something like:

{
  CHECKPOINT("Testing C::insert(value_type&)");
  Container c;
  ValueTp v(42, 1);
  cc->expect<const ValueTp&>(); // <-- !!!!!
  assert(c.insert(v).second);
  assert(!cc->unchecked());
  {
    DisableAllocationGuard g;
    ValueTp v2(42, 1);
    assert(c.insert(v2).second == false);
  }
}

This call to ::insert ends up calling the insert(P&&), not insert(const value_type&), because P is deduced to value_type&, meaning the insert(P&&) overload is an exact match, while insert(const value_type&) requires adding const. See http://eel.is/c++draft/associative#map.modifiers

When the insert(P&&) is called, it delegates to emplace, which only gets Cpp17EmplaceConstructible from the supplied parameters, not Cpp17EmplaceConstructible from add_const_t of the parameters. Thus, libcxx's test is asserting a condition the standard explicitly does not allow at this time. (Though it may be reasonable to fix the standard to make what libcxx is doing okay....)

Unfortunately with the tests fixed to assert the correct allocator::construct call, libc++ will fail these tests. I'm looking for guidance on how libc++ maintainers would like to handle this.

[ldionne]

When the insert(P&&) is called, it delegates to emplace, which only gets Cpp17EmplaceConstructible from the supplied parameters, not Cpp17EmplaceConstructible from add_const_t of the parameters. Thus, libcxx's test is asserting a condition the standard explicitly does not allow at this time. [...]

Agreed.

Unfortunately with the tests fixed to assert the correct allocator::construct call, libc++ will fail these tests. I'm looking for guidance on how libc++ maintainers would like to handle this.

Do you know *why* the tests are failing with libc++? I see this overload for insert and it seems like it should be a better match?

template <class _Pp,
          class = typename enable_if<is_constructible<value_type, _Pp>::value>::type>
    _LIBCPP_INLINE_VISIBILITY
    pair<iterator, bool> insert(_Pp&& __p);

I think we should just fix libc++ to do the right thing (according to the standard).

Separately, I'm not sure it's worth changing the Standard to ensure that the T const& overload is selected -- I don't see a huge benefit here.

[BillyONeal]

In D61364#1502172, @ldionne wrote:

Do you know *why* the tests are failing with libc++? I see this overload for insert and it seems like it should be a better match?

No, I haven't investigated. I avoid looking at libc++ product code too much for now because I want to avoid any appearance or reality of licensing conflicts... for now ;) .

In D61364#1502172, @ldionne wrote:

I think we should just fix libc++ to do the right thing (according to the standard).

That's what I'd recommend :)

In D61364#1502172, @ldionne wrote:

Separately, I'm not sure it's worth changing the Standard to ensure that the T const& overload is selected -- I don't see a huge benefit here.

Would be a code size win if both overloads are called in the same program.

[EricWF]

From Billy and my last discussion, I think we came to the agreement that it's not clear exactly what the "standard behavior" is.

[EricWF]

test/std/containers/map_allocator_requirement_test_templates.h

236

I really think the current behavior libc++ has here is correct.

[BillyONeal]

In D61364#1502200, @EricWF wrote:

From Billy and my last discussion, I think we came to the agreement that it's not clear exactly what the "standard behavior" is.

No, I don't think so. I think there was agreement that the behavior in the standard may not have been what was intended by the original move semantics proposals, but I don't think there's any doubt about what the standard currently requires.

[BillyONeal]

test/std/containers/map_allocator_requirement_test_templates.h

236

File a DR to retroactively make libc++'s behavior standard then? :)

[EricWF]

Fixing this requires a bunch of container overall in the containers that I don't have the time for right now.
So I'm going to go ahead and close this for being inactive.

test/std/containers/map_allocator_requirement_test_templates.h

assert(!cc->unchecked());
       const ValueTp v2(42, 1);
       assert(c.insert(v2).second == false);
     }
   }
   {
     CHECKPOINT("Testing C::insert(value_type&)");
     Container c;
     ValueTp v(42, 1);
-    cc->expect<const ValueTp&>();
+    cc->expect<ValueTp&>();
     assert(c.insert(v).second);
     assert(!cc->unchecked());
     {
       DisableAllocationGuard g;
       ValueTp v2(42, 1);
       assert(c.insert(v2).second == false);
     }
   }
assert(!cc->unchecked());
       ValueTp v2(42, 1);
       assert(c.insert(std::move(v2)).second == false);
     }
   }
   {
     CHECKPOINT("Testing C::insert(const value_type&&)");
     Container c;
     const ValueTp v(42, 1);
-    cc->expect<const ValueTp&>();
+    cc->expect<const ValueTp&&>();
     assert(c.insert(std::move(v)).second);
     assert(!cc->unchecked());
     {
       DisableAllocationGuard g;
       const ValueTp v2(42, 1);
       assert(c.insert(std::move(v2)).second == false);
     }
   }
assert(!cc->unchecked());
       c.insert(std::move_iterator<ValueTp*>(std::begin(ValueList2)),
                std::move_iterator<ValueTp*>(std::end(ValueList2)));
     }
   }
   {
     CHECKPOINT("Testing C::insert(Iter, Iter) for *Iter = value_type&");
     Container c;
     ValueTp ValueList[] = { ValueTp(1, 1), ValueTp(2, 1) , ValueTp(3, 1) };
-    cc->expect<ValueTp const&>(3);
+    cc->expect<ValueTp&>(3);
     c.insert(std::begin(ValueList), std::end(ValueList));
     assert(!cc->unchecked());
     {
       DisableAllocationGuard g;
       c.insert(std::begin(ValueList), std::end(ValueList));
     }
   }
 }
assert(!cc->unchecked());
       assert(&(*ret2) == &(*ret));
       assert(c.size() == 1);
     }
   }
   {
     CHECKPOINT("Testing C::insert(p, value_type&)");
     Container c;
     ValueTp v(42, 1);
-    cc->expect<ValueTp const&>();
+    cc->expect<ValueTp&>();
     It ret = c.insert(c.end(), v);
     assert(ret != c.end());
     assert(c.size() == 1);
     assert(!cc->unchecked());
     {
       DisableAllocationGuard g;
       ValueTp v2(42, 1);
       It ret2 = c.insert(c.begin(), v2);
assert(!cc->unchecked());
       assert(&(*ret2) == &(*ret));
       assert(c.size() == 1);
     }
   }
   {
     CHECKPOINT("Testing C::insert(p, const value_type&&)");
     Container c;
     const ValueTp v(42, 1);
-    cc->expect<const ValueTp&>();
+    cc->expect<const ValueTp&&>();
     It ret = c.insert(c.end(), std::move(v));
     assert(ret != c.end());
     assert(c.size() == 1);
     assert(!cc->unchecked());
     {
       DisableAllocationGuard g;
       const ValueTp v2(42, 1);
       It ret2 = c.insert(c.begin(), std::move(v2));

test/std/containers/set_allocator_requirement_test_templates.h

assert(!cc->unchecked());
       c.insert(std::move_iterator<ValueTp*>(std::begin(ValueList2)),
                std::move_iterator<ValueTp*>(std::end(ValueList2)));
     }
   }
   {
     CHECKPOINT("Testing C::insert(Iter, Iter) for *Iter = value_type&");
     Container c;
     ValueTp ValueList[] = { ValueTp(1), ValueTp(2) , ValueTp(3) };
-    cc->expect<ValueTp const&>(3);
+    cc->expect<ValueTp&>(3);
     c.insert(std::begin(ValueList), std::end(ValueList));
     assert(!cc->unchecked());
     {
       DisableAllocationGuard g;
       c.insert(std::begin(ValueList), std::end(ValueList));
     }
   }
 }