-
Notifications
You must be signed in to change notification settings - Fork 247
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
WINRT_SOURCE_LOCATION has ODR checks that prevent mixing cpp17 and cpp20 static libraries #1444
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonwis
previously approved these changes
Nov 1, 2024
dmachaj
commented
Nov 1, 2024
DefaultRyan
reviewed
Nov 1, 2024
ChrisGuzak
previously approved these changes
Nov 2, 2024
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.
I'm in favor of the macro cleanup too.
CaseyCarter
reviewed
Nov 2, 2024
CaseyCarter
approved these changes
Nov 4, 2024
DefaultRyan
approved these changes
Nov 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why is this change being made?
Someone is trying to upgrade the cppwinrt version used by a large project that has many static libraries using cppwinrt. Some binaries in that project are mixing static libs with different cpp language versions. (This seems like not a great idea generally but it is the state of the world so to some degree we have to live with it). In those binaries the ODR checks for cppwinrt source_location usage are breaking the build. For good reason, it is not safe to mix this functionality across language versions. This set of changes is aimed at making that upgrade process easier without losing any useful functionality.
Briefly summarize what changed
We already have
winrt::impl::slim_source_location
which is a lot likestd::source_location
, minus always containing the very impactful FUNCTION data. This type is powered by the same intrinsics as the STL version so it works as well as the STL library. The existence of this class means that we can avoid the ODR violations by always using winrt::impl::slim_source_location.Some new macros are used to control what goes into the constructor. cpp20 code that does not suppress source_location will get valid source information passed in. Code that is cpp17, or suppresses this feature, will have zero's passed in. Furthermore, when compiling as _DEBUG this will also include the FUNCTION data, matching previous behavior.
The net result is that there is no more ODR violation because the function signatures are always the same.
For now I have elected not to clean up the
WINRT_IMPL_SOURCE_LOCATION_ARGS_NO_DEFAULT
macro family. There is only one option so there is no more ODR violation. But removing that would churn a lot of code. I could be convinced to remove it now but only if there is agreement it is worth it and that we won't try to re-add variance later.How was this change tested?
build_test_all.cmd
for both release and debug. I also created a little project that mixes cpp17 and cpp20 libraries. With the latest public release of cppwinrt this project does not build because of the ODR violations. With these changes it builds and runs as expected.build break with existing cppwinrt
cpp17 code
cpp20 code
console exe code
Release output:
Debug output: