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.zone.leap.overview] Fix example #6383

Merged
merged 2 commits into from Nov 8, 2023
Merged

Conversation

hewillk
Copy link
Contributor

@hewillk hewillk commented Jul 16, 2023

leap_seconds can only be compared with sys_time<Duration>, so we are in nondeduced context here?

Correct me if I'm wrong.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 8, 2023

Does the example compile?

@hewillk
Copy link
Contributor Author

hewillk commented Nov 8, 2023

CC @HowardHinnant

@HowardHinnant
Copy link

The example as corrected compiles. The uncorrected code does not.

@HowardHinnant
Copy link

nitpick: I use {} for construction when possible to disambiguate it visually from a function call which uses ().

source/time.tex Outdated Show resolved Hide resolved
Co-authored-by: Thomas Köppe <tkoeppe@google.com>
@jensmaurer
Copy link
Member

I do want to point out that the standard library specifications try to avoid brace-initialization, due to bad experiences with narrowing prohibitions getting into the way. That doesn't necessarily affect examples, though.

@jwakely , any opinion for this particular case?

Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to follow that specification policy in examples, and this conversion is guaranteed to work without narrowing so the braces are fine.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 8, 2023

I was mostly noting that a existing examples in this area also use braces, so I wasn't concerned by making this locally consistent. That does not constitute endoresement of a wider policy, and I'd be happy to discuss this in a wider context.

@tkoeppe tkoeppe merged commit 62e33ca into cplusplus:main Nov 8, 2023
2 checks passed
@hewillk hewillk deleted the main-leap branch November 8, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants