Re: [v3 PATCH] Implement C++17 GB50 resolution

[Dinka Ranns <dinka dot ranns at googlemail dot com>]

Comments addressed. Please find the new diff attached to this e-mail.

Changelog after review comments :

2017-02-18 Dinka Ranns <dinka.ranns@googlemail.com>

        C++17 GB50 resolution
        * include/std/chrono:
        (duration::operator++()): Add _GLIBCXX17_CONSTEXPR.
        (duration::operator++(int)): Likewise
        (duration::operator--()): Likewise
        (duration::operator--(int)): Likewise
        (duration::operator+=(const duration&)): Likewise
        (duration::operator-=(const duration&)): Likewise
        (duration::operator*=(const rep&)): Likewise
        (duration::operator/=(const rep&)): Likewise
        (duration::operator%=(const rep&)): Likewise
        (duration::operator%=(const duration&)): Likewise
        (time_point::operator+=(const duration&)): Likewise
        (time_point::operator-=(const duration&)): Likewise

        * testsuite/20_util/duration/arithmetic/constexpr_c++17.cc: new tests
        * testsuite/20_util/time_point/arithmetic/constexpr.cc: new tests




On 15 February 2017 at 09:35, Jonathan Wakely <jwakely@redhat.com> wrote:
> Hi, Dinka, thanks for the patch.
>
> On 14/02/17 21:22 +0000, Dinka Ranns wrote:
>>
>> diff --git a/libstdc++-v3/include/std/chrono
>> b/libstdc++-v3/include/std/chrono
>> index ceae7f8..6a6995c 100644
>> --- a/libstdc++-v3/include/std/chrono
>> +++ b/libstdc++-v3/include/std/chrono
>> @@ -349,50 +349,50 @@ _GLIBCXX_END_NAMESPACE_VERSION
>>         operator-() const
>>         { return duration(-__r); }
>>
>> -       duration&
>> +       constexpr duration&
>
>
> This needs to use _GLIBCXX17_CONSTEXPR instead of 'constexpr'
>
> These functions aren't constexpr in C++11 and C++14, and the standard
> (annoyingly) forbids us from adding constexpr anywhere it isn't
> present in the standard.
>
> The macro _GLIBCXX17_CONSTEXPR expands to 'constexpr' if __cplusplus >
> 201402L, and expands to nothing otherwise.
>
> Each new 'constexpr' you've added needs to use that macro.
>
>> diff --git
>> a/libstdc++-v3/testsuite/20_util/duration/arithmetic/constexpr.cc
>> b/libstdc++-v3/testsuite/20_util/duration/arithmetic/constexpr.cc
>> index 285f941..1128a52 100644
>> --- a/libstdc++-v3/testsuite/20_util/duration/arithmetic/constexpr.cc
>> +++ b/libstdc++-v3/testsuite/20_util/duration/arithmetic/constexpr.cc
>> @@ -19,11 +19,31 @@
>>
>> #include <chrono>
>> #include <testsuite_common_types.h>
>
>
> There should be a blank line before and after this function, however
> ...
>
>> +constexpr auto test_operators()
>> +{
>> +  std::chrono::nanoseconds d1 { };
>> +  d1++;
>
>
> This new function uses C++14 return type deduction, so will fail if
> the test is run in C++11 mode (the default is C++14, but it can be
> overridden on the command-line).
>
>> +  ++d1;
>> +  d1--;
>> +  --d1;
>
>
> Also once you change the new 'constexpr' specifiers to use the
> _GLIBCXX17_CONSTEXPR macro this test will fail in C++14 mode. I think
> this new function needs to be moved to a new test file, such as
> testsuite/20_util/duration/arithmetic/constexpr_c++17.cc
>
> That should contain just your new test_operators() function, because
> the rest of the class will be tested by the existing constexpr.cc test
> file. So something like:
>
> // { dg-options "-std=gnu++17" }
> // { dg-do compile { target c++1z } }
>
> // Copyright etc. etc.
> // ...
>
> #include <chrono>
>
> constexpr auto test_operators()
> {
>  // ...
> }
>
> constexpr auto d4 = test_operators();
>
>
> Note that the "dg-do compile" line should use the c++1z target instead
> of c++11, and needs to override the default dialect with a dg-options
> line.
>
> This test doesn't need a "main" function because it's a "dg-do
> compile" test, so isn't linked. (The existing test that you modified
> didn't need one either, but it doesn't do any harm leaving it there).
>
>
>> diff --git
>> a/libstdc++-v3/testsuite/20_util/time_point/arithmetic/constexpr.cc
>> b/libstdc++-v3/testsuite/20_util/time_point/arithmetic/constexpr.cc
>> new file mode 100644
>> index 0000000..e87a226
>> --- /dev/null
>> +++ b/libstdc++-v3/testsuite/20_util/time_point/arithmetic/constexpr.cc
>> @@ -0,0 +1,39 @@
>> +// { dg-do compile { target c++11 } }
>
>
> Since the time_point member functions will only be constexpr in C++17
> this test also needs to use c++1z instead of c++11, and needs to
> override the default C++14 dialect, i.e.
>
> // { dg-options "-std=gnu++17" }
> // { dg-do compile { target c++1z } }
>

Attachment: gb50_changes_v2.diff
Description: Text document