This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] chrono::month_weekday should not be default constructible
ClosedPublic

Authored by CaseyCarter on Jan 18 2022, 10:49 PM.

Details

Summary

It was not in P0355R7, nor has it ever been so in a working draft.

Drive-by:

  • tests should test something: fix loop bounds so initial value is not >= final value
  • calender type streaming tests are nearly useless - let's remove them
  • don't declare printf, especially if you don't intend to use it

Diff Detail

Event Timeline

CaseyCarter requested review of this revision.Jan 18 2022, 10:49 PM
CaseyCarter created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 18 2022, 10:49 PM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/test/std/utilities/time/time.cal/time.cal.month/time.cal.month.nonmembers/comparisons.pass.cpp
42–45

For test coverage this should say

for (int i=1; i <= 12; ++i)
    for (int j=1; j <= 12; ++j)
        assert(testComparisons6Values<month>(i, j));

(I wildly speculate that it originally ran from 1 to 10 in both loops, but that that didn't pass because all this stuff was broken, so someone changed j=1 to j=10 to nerf the test and then shipped it.)

libcxx/test/std/utilities/time/time.cal/time.cal.wdidx/time.cal.wdidx.nonmembers/streaming.pass.cpp
0

You missed this XFAIL: *.
But, looking at these XFAIL'ed tests, I'm noticing that none of them are good. They just print something to std::cout (!) and then exit without asserting anything (!!). Do we want to just delete them, and ask whoever eventually implements <chrono> to write their own tests?

libcxx/test/std/utilities/time/time.cal/time.cal.weekday/time.cal.weekday.nonmembers/minus.pass.cpp
32–42

Thanks for working on this!

libcxx/include/__chrono/calendar.h
543

I think this should be mentioned in the release notes. libcxx/docs/ReleaseNotes.rst

libcxx/test/std/utilities/time/time.cal/time.cal.wdidx/time.cal.wdidx.nonmembers/streaming.pass.cpp
0

I think that makes sense. These tests only give a false impression we've implemented these parts of chrono.

CaseyCarter marked an inline comment as done.
CaseyCarter edited the summary of this revision. (Show Details)

Address review comments.

CaseyCarter marked an inline comment as done.Jan 19 2022, 10:51 AM
CaseyCarter added inline comments.
libcxx/test/std/utilities/time/time.cal/time.cal.month/time.cal.month.nonmembers/comparisons.pass.cpp
42–45

Ah, I incorrectly assumed from the fact that the ranges of i and j were disjoint that testComparisons6Values expected its first argument to be less than the second. Fixing, but I'll write < 13 to be consistent with every other loop in these tests.

libcxx/test/std/utilities/time/time.cal/time.cal.weekday/time.cal.weekday.nonmembers/minus.pass.cpp
32–42

Missed this comment. Fix incoming.

CaseyCarter marked an inline comment as done.
CaseyCarter edited the summary of this revision. (Show Details)

Address missed review comment.

libcxx/test/std/utilities/time/time.cal/time.cal.month/time.cal.month.nonmembers/comparisons.pass.cpp
42–45

but I'll write < 13

Well, any choice can hardly make this test worse, so whatever. ;)
But for the record, I'm confident in my preference for numbering the calendar months 1<=x<=12 instead of 1<=x<13. ;) (Unless this loop is flat-out wrong and it should run 0<=x<12? That's not the case, right? I'm like 75% sure that months are 1-indexed and month 0 is UB or something like that.)

CaseyCarter marked an inline comment as done.

Use [1, 12] uniformly for months.

Mordante accepted this revision.Jan 20 2022, 10:00 AM

LGTM, with one request.

libcxx/test/std/utilities/time/time.cal/time.cal.ym/time.cal.ym.nonmembers/comparisons.pass.cpp
61

Please fix this indention before landing, same applies to the tests below.

This revision is now accepted and ready to land.Jan 20 2022, 10:00 AM
CaseyCarter marked an inline comment as done.Jan 20 2022, 11:53 AM
CaseyCarter added inline comments.
libcxx/test/std/utilities/time/time.cal/time.cal.ym/time.cal.ym.nonmembers/comparisons.pass.cpp
61

Done - I audited everything under test/std/utilities/time.cal.

libcxx/test/std/utilities/time/time.cal/time.cal.ymwdlast/time.cal.ymwdlast.nonmembers/comparisons.pass.cpp