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

Extract platform and feature detection to a separate file and add macro support for a number of C++20/23 features #93

Merged
merged 25 commits into from
May 7, 2024

Conversation

Rinzii
Copy link
Contributor

@Rinzii Rinzii commented May 1, 2024

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!

@Rinzii Rinzii marked this pull request as draft May 1, 2024 22:03
@Rinzii Rinzii changed the title Add full C++20 & C++23 compatibility layer Add a full C++20 & C++23 compatibility layer May 1, 2024
@Rinzii Rinzii marked this pull request as ready for review May 2, 2024 00:46
@Rinzii
Copy link
Contributor Author

Rinzii commented May 2, 2024

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.

Copy link
Owner

@jeremy-rifkin jeremy-rifkin left a 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.

.gitignore Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
include/libassert/libassert_internal.hpp Outdated Show resolved Hide resolved
@Rinzii
Copy link
Contributor Author

Rinzii commented May 2, 2024

Applied requested changes and also added in a few more items that I found were missing.

@Rinzii Rinzii requested a review from jeremy-rifkin May 2, 2024 04:19
@Rinzii
Copy link
Contributor Author

Rinzii commented May 2, 2024

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:

  • libassert internally sets its compile option to C++17.
  • The target demo then sets its compile options to C++20.
  • The preprocessor sees C++20 and then copy and pastes the correct items for C++20.
  • Then the linker trys to perform linking but instead sees C++17 for some reason.
  • This causes a linker error and the project fails.

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?

@jeremy-rifkin
Copy link
Owner

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.

@Rinzii
Copy link
Contributor Author

Rinzii commented May 3, 2024

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.

@jeremy-rifkin
Copy link
Owner

Thank you 🙏

@Rinzii
Copy link
Contributor Author

Rinzii commented May 4, 2024

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

Copy link
Owner

@jeremy-rifkin jeremy-rifkin left a 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!

CMakeLists.txt Outdated Show resolved Hide resolved
include/libassert/assert.hpp Outdated Show resolved Hide resolved
include/libassert/platform.hpp Outdated Show resolved Hide resolved
include/libassert/platform.hpp Outdated Show resolved Hide resolved
include/libassert/platform.hpp Outdated Show resolved Hide resolved
include/libassert/platform.hpp Outdated Show resolved Hide resolved
@Rinzii Rinzii requested a review from jeremy-rifkin May 5, 2024 19:07
Copy link
Owner

@jeremy-rifkin jeremy-rifkin left a 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

.github/workflows/build.yml Outdated Show resolved Hide resolved
include/libassert/assert.hpp Outdated Show resolved Hide resolved
include/libassert/platform.hpp Outdated Show resolved Hide resolved
@jeremy-rifkin
Copy link
Owner

MSVC is breaking because of using __VA_OPT__ https://godbolt.org/z/P16d3Gr5c

@jeremy-rifkin
Copy link
Owner

Since this doesn't appear to be an issue you introduced I'm going to go ahead and merge and fix on dev

@jeremy-rifkin jeremy-rifkin changed the title Add a full C++20 & C++23 compatibility layer Extract platform and feature detection to a separate file and add macro support for a number of C++20/23 features May 7, 2024
@jeremy-rifkin jeremy-rifkin merged commit 71617b4 into jeremy-rifkin:dev May 7, 2024
41 of 44 checks passed
@jeremy-rifkin
Copy link
Owner

Oh, it was introduced here due to #define LIBASSERT_CPLUSPLUS _MSVC_LANG. But we can workaround that.

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.

2 participants