Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[time.cal] improve std::chrono::month operator+'s description of arithmetic mod 12 #1974

Open
zygoloid opened this issue Mar 23, 2018 · 8 comments · May be fixed by #1999
Open

[time.cal] improve std::chrono::month operator+'s description of arithmetic mod 12 #1974

zygoloid opened this issue Mar 23, 2018 · 8 comments · May be fixed by #1999
Labels
lwg Issue must be reviewed by LWG.

Comments

@zygoloid
Copy link
Member

Perhaps we could just say:

Returns: a month with m_ equal to the sum of unsigned{x} and y.count(), reduced modulo 12 to an integer in [1, 12].

or something like that, rather than defining an auxiliary modulo(n, 12) function and going on an excursion to a definition of Euclidean division.

One problem: this is not entirely editorial, as the new wording does not admit any undefined behavior, whereas the description in the existing wording results in UB when long long and months' representation type are the same size and the months value is nearly LLONG_MAX. The undefined behavior here does not appear to be especially useful; implementations can easily produce the correct result by reducing the months value modulo 12 first, if they chose a representation type for months that is the same size as long long.

@jensmaurer
Copy link
Member

This seems to be related to the pending motions. There is no class "month" in the current working draft.

@jensmaurer jensmaurer added the after-motions Pull request is to be applied after the pending edits from WG21 straw polls have been applied. label Mar 23, 2018
@zygoloid
Copy link
Member Author

The "Euclidean division" description is repeated in the description of std::chrono::weekday, and any fix should be applied there too.

@jensmaurer jensmaurer self-assigned this Mar 29, 2018
@jensmaurer jensmaurer added lwg Issue must be reviewed by LWG. and removed after-motions Pull request is to be applied after the pending edits from WG21 straw polls have been applied. labels Mar 29, 2018
@HowardHinnant
Copy link

I'm kind of fond of the specification in terms of Euclidean division. I believe it says precisely what we want it to say. However your point about overflow is well taken.

One possible solution is to add to [time.cal.general] something along the lines of:

Any calendrical computation that would result in a value that is outside of the
range [year::min()/January/1, year::max()/December/31] has an unspecified result.

That's probably not exactly right. But something in that direction...

@jensmaurer
Copy link
Member

jensmaurer commented Apr 13, 2018

@HowardHinnant, what exactly do you fear will be lost when we talk about "reduced modulo y" instead? That seems to be fully equivalent, and one could argue that explicitly specifying "Euclidian division" might actually be overspecification (it's purporting to prescribe an algorithm, not a result.)

@jensmaurer jensmaurer changed the title improve std::chrono::month operator+'s description of arithmetic mod 12 [time.cal] improve std::chrono::month operator+'s description of arithmetic mod 12 Apr 13, 2018
@HowardHinnant
Copy link

I see two problems:

  1. "reduced modulo 12" doesn't result in a number in the range [1, 12].

  2. I fear that "reduced modulo 12" will be coded as n % 12 which gives the wrong answer when n < 0.

I'm using this terminology for modulo and it must give a result in the range [0, 11] for negative and non-negative n. Floored division would work just as well as Euclidean division. But truncated division (which is what the C++ % operator does) gives the wrong answer for negative n.

@jensmaurer
Copy link
Member

@HowardHinnant, thanks for your thoughts. I believe we very carefully use English, not C++ code, to describe the modulo operation here, which is thus clearly highlighted as a mathematical operation, not a C++ operation. All we're doing is getting the canonical representative for the equivalence class, and we clearly say these are defined to be in [1,12] in our case.
Regarding your second concern, such an implementation would clearly violate any conceivable specification of the semantics (and the standard is not a tutorial), so I disagree we need the verbose description for that purpose.

@HowardHinnant
Copy link

I'm happy for the LWG to explore alternative wording, and to help review said wording. The most important thing for me is that vendors know what to provide and that customers understand what they are getting.

I would prefer that this change not be done editorially, but through a LWG issue so that it benefits from a full review of the LWG.

@jensmaurer jensmaurer removed their assignment Apr 13, 2018
@zygoloid
Copy link
Member Author

Perhaps something like:

Returns: a month with m_ equal to an integer in [1,12] that is congruent to the sum of unsigned{x} and y.count() modulo 12.

I think that should completely avoid the "number between 0 and 11" interpretation.

Just some food for thought for the LWG discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lwg Issue must be reviewed by LWG.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants