Bug 54025 - atomic<chrono::duration> won't compile: chrono::duration::duration() is not C++11 compliant
Summary: atomic<chrono::duration> won't compile: chrono::duration::duration() is not C...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.8.0
: P3 normal
Target Milestone: 4.8.0
Assignee: Not yet assigned to anyone
URL:
Keywords: rejects-valid
Depends on:
Blocks:
 
Reported: 2012-07-19 02:55 UTC by Chip Salzenberg
Modified: 2012-07-20 09:51 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-07-19 00:00:00


Attachments
patch to duration default ctor (235 bytes, patch)
2012-07-19 02:56 UTC, Chip Salzenberg
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chip Salzenberg 2012-07-19 02:55:28 UTC
Attempting to compile atomic<duration> fails, because the duration default constructor is not " = default" as required by the standard, but instead explicitly initializes its representation.  Here is what libstdc++ says:

    constexpr duration() : __r() { }

but here is what the standard says should be there, and if I make the change, compilation succeeds:

    constexpr duration() = default;

Test source:

#include <atomic>
#include <chrono>
using namespace std;
using namespace chrono;
int main() {
    atomic<duration<long, micro>> dur;
}

Error before patch:

/usr/include/c++/4.7/atomic: In instantiation of ‘struct std::atomic<std::chrono::duration<long int, std::ratio<1ll, 1000000ll> > >’:
atdur.cc:6:35:   required from here
/usr/include/c++/4.7/atomic:160:7: error: function ‘std::atomic<_Tp>::atomic() [with _Tp = std::chrono::duration<long int, std::ratio<1ll, 1000000ll> >]’ defaulted on its first declaration with an exception-specification that differs from the implicit declaration ‘constexpr std::atomic<std::chrono::duration<long int, std::ratio<1ll, 1000000ll> > >::atomic()’
Comment 1 Chip Salzenberg 2012-07-19 02:56:57 UTC
Created attachment 27829 [details]
patch to duration default ctor
Comment 2 Jonathan Wakely 2012-07-19 08:34:03 UTC
confirmed
Comment 3 Paolo Carlini 2012-07-19 09:40:07 UTC
Jon, shall I just apply patch and testcase?
Comment 4 Jonathan Wakely 2012-07-19 10:20:07 UTC
Yes, I think so.  I initially wondered if there was some interaction with PR 53901 but I think that just makes the bug visible, duration's constructor should be explicitly-defaulted anyway.
Comment 5 Paolo Carlini 2012-07-19 11:57:04 UTC
If we just do the change and nothing else, 20_util/duration/cons/constexpr.cc doesn't compile anymore with this error:

/scratch/Gcc/svn-dirs/trunk/libstdc++-v3/testsuite/20_util/duration/cons/constexpr.cc:27:42:   required from here
/scratch/Gcc/svn-dirs/trunk/libstdc++-v3/testsuite/util/testsuite_common_types.h:698:18: error: uninitialized const '__obj' [-fpermissive]
  { constexpr _Tp __obj; }
                  ^
In file included from /scratch/Gcc/svn-dirs/trunk/libstdc++-v3/testsuite/20_util/duration/cons/constexpr.cc:21:0:
/home/paolo/Gcc/svn-dirs/trunk-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/chrono:220:14: note: 'const struct std::chrono::duration<long int>' has no user-provided default constructor
       struct duration
              ^
/home/paolo/Gcc/svn-dirs/trunk-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/chrono:231:12: note: constructor is not user-provided because it is explicitly defaulted in the class body
  constexpr duration() = default;
            ^
/home/paolo/Gcc/svn-dirs/trunk-build/x86_64-unknown-linux-gnu/libstdc++-v3/include/chrono:349:6: note: and the implicitly-defined constructor does not initialize 'std::chrono::duration<long int>::rep std::chrono::duration<long int>::__r'
  rep __r;
      ^
Comment 6 Jonathan Wakely 2012-07-19 12:06:32 UTC
I think that test is wrong, a duration is only constexpr_default_constructible if the rep type has a default-constructor, but std::chrono::seconds uses a scalar rep.

I think the test should use something like this to check duration is constexpr-default-constructible:

struct Seconds {
    constexpr Seconds() = default;
    std::chrono::seconds s{};
};
test1.operator()<std::chrono::duration<Seconds>>();
Comment 7 Jonathan Wakely 2012-07-19 12:08:29 UTC
(In reply to comment #6)
> I think that test is wrong, a duration is only constexpr_default_constructible
> if the rep type has a default-constructor

... that initializes all its members.
Comment 8 Paolo Carlini 2012-07-19 12:17:13 UTC
Understood, thanks. A couple of times in the past I already tweeked a bit some of those tests. Let me see...
Comment 9 Jonathan Wakely 2012-07-19 12:19:05 UTC
Another option would be to give duration::__rep a NSDMI, which leaves the testcase valid (as a QoI feature, I believe)
Comment 10 Paolo Carlini 2012-07-19 12:23:40 UTC
I see. For now I would rather just minimally tweak to testcase. If you want to play a bit more with this and experiment with the various options, just let me know.
Comment 11 Jonathan Wakely 2012-07-19 12:34:19 UTC
Oh, I think our current code might be intentional, we should ask Benjamin:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3303.html

See c++std-lib-32464 for rational for the std semantics i.e. duration<trivial_type> is a trivial type, and leaves the rep uninitialized.
Comment 12 Paolo Carlini 2012-07-19 12:48:19 UTC
Ah! Let's add in CC both Benjamin and Daniel then.
Comment 13 Daniel Krügler 2012-07-19 15:18:16 UTC
(In reply to comment #12)
> Ah! Let's add in CC both Benjamin and Daniel then.

I more and more tend to change my mind: I recommend to ignore the recommendation of N3303 and to make duration conforming with the current library spec. The described use-case here makes very much sense to me and demonstrates to me that N3303 did overreact here. But I would be happy to hear Benjamin's position as well.
Comment 14 Paolo Carlini 2012-07-19 15:25:39 UTC
Thanks Daniel. Therefore let's wait a bit in case Benjamin disagrees, otherwise I'll take care of applying patchlet + testcase + tweak to the existing constexpr testcase,
Comment 15 paolo@gcc.gnu.org 2012-07-20 09:49:02 UTC
Author: paolo
Date: Fri Jul 20 09:48:57 2012
New Revision: 189711

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=189711
Log:
2012-07-20  Chip Salzenberg  <chip@pobox.com>
	    Jonathan Wakely  <jwakely.gcc@gmail.com>

	PR libstdc++/54025
	* include/std/chrono (duration<>::duration): Fix per C++11.
	* testsuite/20_util/duration/cons/54025.cc: New.
	* testsuite/20_util/duration/cons/constexpr.cc: Adjust.

Added:
    trunk/libstdc++-v3/testsuite/20_util/duration/cons/54025.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/chrono
    trunk/libstdc++-v3/testsuite/20_util/duration/cons/constexpr.cc
Comment 16 Paolo Carlini 2012-07-20 09:51:39 UTC
Done.