-
Notifications
You must be signed in to change notification settings - Fork 41
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
Extract platform and feature detection to a separate file and add macro support for a number of C++20/23 features #93
Conversation
My first initial revision of what this would look like has been finished. I would though like to request permission to add C++20 and C++23 to the CI so we can properly test against this. |
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.
Thanks for the PR! I have left some comments.
Applied requested changes and also added in a few more items that I found were missing. |
I've come across a strange error with the current approach to implementing the use of both an internal source_location and std::source_location. It's an issue I've never experienced before. Currently the preprocessor gets mega confused with the project with how one would traditionally switch between two implementations with macros if one target is compiling for C++20 but another target is compiling for C++17. It appears this trips up the preprocessor and causes a link time error. What is happening is essentially as follows:
If both targets are using the same C++ version then this issue does not occur. Some of this is speculation based on what I've observed through debugging and its a strange issue. I'm not 100% sure what would be the best way to approach it without doing some compilation checking through cmake or something to verify what C++ version we are using, but this would be much more work. What are your thoughts on this issue? |
I haven’t read the new changes yet but this sounds expected. If you conditionally enable something based on the standard and that makes changes in the compiled library then it won’t be compatible with code built with a different standard. You’re lucky you got a linker error, this can easily be a nasty ODR violation that doesn’t manifest until it causes a segfault :P I see no reason to go out of the way to use newer C++ features like this. The C++17 version works and I’m not sure I see the benefit in this. It can only lead to complexity, code duplication, and ODR problems. I’m happy to support newer things if it makes a difference for end-users, though. |
That is fine. I'll make a final adjustment and get everything together. |
Thank you 🙏 |
For proof of concept on the C++26's user generated static_assert you can see it in action here: https://godbolt.org/z/hY7fz5rKG For info on what it is here is the specification: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2741r1.pdf |
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.
Thanks again so much for your interest in contributing and all the patience in working through my comments, I have a few more (mostly minor syntax things), but this is looking good!
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.
Thanks for the changes! A couple more quick comments
MSVC is breaking because of using |
Since this doesn't appear to be an issue you introduced I'm going to go ahead and merge and fix on dev |
Oh, it was introduced here due to |
This PR focuses on building the infrastructure to allow the use of modern C++20 and C++23 features if available. Implementing the library features into the project is outside this PR's scope. Instead, this PR is focused on laying the foundation so that we can gradually add newer features to the library while still maintaining backward support with C++17!