[libcxx-commits] [PATCH] D117638: [libcxx] chrono::month_weekday should not be default constructible

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/utilities/time/time.cal/time.cal.month/time.cal.month.nonmembers/comparisons.pass.cpp:43-45
     for (unsigned i = 1; i < 10; ++i)
-        for (unsigned j = 10; j < 10; ++j)
+        for (unsigned j = 10; j < 13; ++j)
             assert(testComparisons6Values<month>(i, j));
----------------
CaseyCarter wrote:
> Quuxplusone wrote:
> > 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.)
> 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.
> 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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117638/new/

https://reviews.llvm.org/D117638