Step 2 (of N?) fixing bug 16599

Howard Hinnant hhinnant at apple.com
Tue Jul 16 10:37:55 PDT 2013


On Jul 16, 2013, at 1:32 AM, Marshall Clow <mclow.lists at gmail.com> wrote:

> On Jul 15, 2013, at 4:43 PM, Howard Hinnant <howard.hinnant at gmail.com> wrote:
> 
>> 
>> On Jul 15, 2013, at 5:19 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
>> 
>>> This is the second step towards fixing http://llvm.org/bugs/show_bug.cgi?id=16599
>>> 
>>> Make std::pair's constructors and comparison operators (and make_pair) constexpr.
>> 
>> This is looking great!
>> 
>> I've got just a few minor tweaks, and one big favor... ;-)
>> 
>> While you're in the pair neighborhood, it would be great if you could do a drive-by fix on the pair copy and move constructors concerning making them = default when _LIBCPP_HAS_NO_DEFAULTED_FUNCTIONS is not defined, but keeping the current definitions when _LIBCPP_HAS_NO_DEFAULTED_FUNCTIONS is defined.
> 
> Done.
> 
>> Otherwise I can find nothing at all to complain about in <utility>.
>> 
>> On the tests I think we need to test every constructor we're marking constexpr.  The easiest way to ensure complete coverage is when you think you have it, comment out the constexpr on the constructors one by one and make sure you have a failing test.  utility/pairs/pairs.pair has an attempted test/per constructor.  We should put a constexpr test in each of those files targeting the relevant constructor.
> 
> Done, except for one constructor:
>    template<class _Tuple,
>             class = typename enable_if<__tuple_convertible<_Tuple, pair>::value>::type>
>        _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
>        pair(_Tuple&& __p)
> 
> We don't currently have any tests for this because I haven't done the constexpr stuff for tuple yet.
> 
>> The make_pair test should go in utility/pairs/pair.spec/make_pair.pass.cpp.  And the comparison operator tests should go in utility/pairs/pair.spec/comparison.pass.cpp.  This will add some duplication in the tests, but that way we can find all of the relevant tests when we have a bug report in this area a year from now.
> 
> I think I've got these all in the right place now.
> 
> Updated patch attached.

This looks awesome Marshall, thanks!

Please commit.

I have one minor nit:

In pairs/pairs.pair/assign_const_pair_U_V.pass.cpp, there is an unnecessary test.  This is the same test as the one you have in const_pair_U_V.pass.cpp.  The one in assign_const_pair_U_V.pass.cpp can be removed (svn revert the whole file).

Thanks!

Howard




More information about the cfe-commits mailing list