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

[format.context] Fix incorrect example #6970

Merged
merged 1 commit into from
May 14, 2024

Conversation

hewillk
Copy link
Contributor

@hewillk hewillk commented May 12, 2024

isdigit() is not a contexpr function so it cannot be used in parse(), and the alignment seems to be in the wrong direction.
Demo of the original example: https://godbolt.org/z/qbs6dvq9q
fixed: https://godbolt.org/z/sjWdxxzWc

@hewillk
Copy link
Contributor Author

hewillk commented May 14, 2024

Sorry, I think there is still something wrong with this example, because if (value < 0 || value > numeric_limits<int>::max()) will trigger the warning of comparison of constant '0' with boolean expression is always false: https://godbolt.org/z/5h7TsfdPE
@jwakely, any thoughts?

@frederick-vs-ja
Copy link
Contributor

Sorry, I think there is still something wrong with this example, because if (value < 0 || value > numeric_limits<int>::max()) will trigger the warning of comparison of constant '0' with boolean expression is always false: https://godbolt.org/z/5h7TsfdPE
@jwakely, any thoughts?

The current example is totally correct to me. Not sure whether it's a good idea to make examples in the standard warning-free.

@jwakely
Copy link
Member

jwakely commented May 14, 2024

I agree, we don't need to fix warnings. The bool case works correctly despite the warnings (and isn't actually used at runtime anyway).

@jensmaurer jensmaurer merged commit 9cd8d6c into cplusplus:main May 14, 2024
2 checks passed
@hewillk hewillk deleted the main-format branch May 14, 2024 13:50
jensmaurer pushed a commit to jensmaurer/draft that referenced this pull request Jul 28, 2024
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

4 participants