Page MenuHomePhabricator

Proposed change to the constructor of std::pair
Needs ReviewPublic

Authored by mclow.lists on Feb 3 2015, 11:11 AM.

Details

Reviewers
howard.hinnant
Summary

Eric Niebler proposed this as a fix for http://llvm.org/bugs/show_bug.cgi?id=21917.
I've tested it, and found that it does solve the problem in the PR.

Note that this is (probably) a defect in the standard, and that this would be an extension.

My concern is to make sure that this is (a) a conforming extension, and (b) not an ABI change.
The standard says that the default constructor for pair requires that both types be default-constructible.

I don't have tests for this (yet), but the program in the bug now compiles w/o error.

Diff Detail

Event Timeline

mclow.lists retitled this revision from to Proposed change to the constructor of std::pair.
mclow.lists updated this object.
mclow.lists edited the test plan for this revision. (Show Details)
mclow.lists added reviewers: EricWF, howard.hinnant.
mclow.lists added a subscriber: Unknown Object (MLST).

The full fix is to go through *all* the constructors and assignment operators of pair and tuple and apply the same change. tuple already does this in many places, but it's inconsistent, and pair doesn't do it at all.

More: It's not so much that this is a defect in the standard (the default constructor's requirement that both types must be default constructible is a requirement on the caller, and having them be references, which are not default constructible is clearly an error), but instead an effort to make pair more SFINAE-friendly; to make the default constructor nonexistent instead of a hard error.

Yes and no. It also means getting the answer right for std::is_default_constructible<pair<int&,int&>>::value. Right now, it says "true", when it's more accurate to say "false".

EricWF edited edge metadata.Feb 3 2015, 12:16 PM

The changes aren't c++03 compatible since they use default template arguments in a function template.

This would be an ABI break (changing the default constructor from a plain function to a template) if it wasn't marked as "always inline". This means that the constructor never gets "called" (the code is just splatted in), and hence, no ABI break :-)

Also, for reference, see LWG issue #2367 http://cplusplus.github.com/LWG/lwg-active.html#2367

The changes aren't c++03 compatible since they use default template arguments in a function template.

Right - I *could* put the template args into a pointer argument with a default value

Pretty sure defaulted template parameters are used elsewhere already, are they not? Would adding an argument with a default change is_trivial for pairs?

Pretty sure defaulted template parameters are used elsewhere already, are they not? Would adding an argument with a default change is_trivial for pairs?

If you have

pair(std::enable_if<std::is_default_constructible<_T1>::value && std::is_default_constructible<_T2>::value>::type* = 0);

Then pair will fail to instantiate when the enable_if is false, so you still need the template<bool __b = true> to make it a template and delay instantiation. However this doesn't work in C++03.

You could wrap the templated default ctor in a feature-test macro. This is done many other places in the library.

The code as it currently stands now will fail to compile the following code (that currently works).

#include <type_traits>

template <class T>
struct IllFormedDefaultImp {
    IllFormedDefaultImp(T x) : value(x) {}
    constexpr IllFormedDefaultImp() {}
    T value;
};

typedef IllFormedDefaultImp<int &> IllFormedDefault;

template <class T, class U>
struct pair
{
#if defined(USE_SFINAE)
  template <bool Dummy = true,
    class = typename std::enable_if<
         std::is_default_constructible<T>::value
      && std::is_default_constructible<U>::value
      && Dummy>::type
    >
#endif
  constexpr pair() : first(), second() {}

  pair(T const & t, U const & u) : first(t), second(u) {}

  T first;
  U second;
};

int main()
{
  int x = 1;
  IllFormedDefault v(x);
  pair<IllFormedDefault, IllFormedDefault> p(v, v);
}

clang++ -std=c++11 test.cpp compiles just fine.
clang++ -std=c++11 -DUSE_SFINAE test.cpp fails to compile.

You can work around this by using something like this

template <class T, bool>
struct dependent_is_default_constructible : std::is_default_constructible<T> {};
EricWF resigned from this revision.Jul 17 2015, 7:17 PM
EricWF removed a reviewer: EricWF.

Not sure where this is going. Resigning as reviewer.