-
Notifications
You must be signed in to change notification settings - Fork 11
Add missing header: cstdlib #56
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
Conversation
Also changed the build instruction from -DCMAKE_CXX_STANDARD=20 to -DCMAKE_CXX_STANDARD=23. Signed-off-by: Ted Lyngmo <[email protected]>
| ```text | ||
| # Configure build | ||
| $ cmake -S . -B build -DCMAKE_CXX_STANDARD=20 | ||
| $ cmake -S . -B build -DCMAKE_CXX_STANDARD=23 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the motivation on changing the standard level to 23? I didn't think this implementation required anything in 23 -- except maybe constexpr which probably depends on 26.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeffGarland consteval is the thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok - should that be conditional in the implementation? I mean I get that there's a lot of interest in this at constexpr/eval time -- but lets not forget it's useful for earlier c++ at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will not compile in pre C++23 mode so, C++23 unconditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'm sure with enough macro magic that wouldn't be the case. Obviously with degraded functionality. And to be clear, not trying to be difficult but just explore what's reasonable/possible. Anyway, all good please proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We changed implementation. C++20 support in doc was for the previous implementation, I must have missed this when switching implementation. I will explore this after #55 is done.
|
Thanks! |
wusatosi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
Also changed the build instruction from
-DCMAKE_CXX_STANDARD=20to-DCMAKE_CXX_STANDARD=23.