Remove ABI-incompatible assertion code in any_view.hpp #1816
Conversation
This file contains hidden or 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
If a provider of an any_view is built without NDEBUG defined, but a consumer is built with NDEBUG defined, the consumer will crash due to ABI-incompatibility in any_ref. Repros on clang with sanitizers, and that's an easily shareable demo: https://godbolt.org/z/Go9qxnjrM Can't get it to repro with a small example in gcc, even with sanitizers Repros in a small example on windows with msvc using same cpp/hpp files as above link - usually assertion failure in the assertion that this commit removes, sometimes Access Violation, but always crashes one way or another. No sanitizers needed to get a crash with msvc. It's just not easily shareable.
ericniebler
reviewed
Sep 3, 2024
This reverts commit fbfb4c3.
|
Nice, thank you for picking this up! Can't say I've needed to sort out how to suppress a warning for a sometimes-unused field before, but now I know how 😄 |
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.
If a provider of an any_view is built without NDEBUG defined, but a consumer is built with NDEBUG defined (e.g., developing one project but using "release"/installed version of another), the consumer will crash due to ABI-incompatibility in any_ref.
Using "remove the assertion" as a discussion-starter, at least. I'd also understand an argument that some might want the ability to keep this assertion. If that's the case, could do something like what llvm does to separate ABI-breaking checks from non-ABI-breaking checks.
Or if we're good with just removing this assertion that works of course (as far as I can tell, it's the only ABI-breaking assertion in range-v3)
Repros on clang with sanitizers, and that's an easily shareable demo:
https://godbolt.org/z/Go9qxnjrM
Can't get it to repro with a small example in gcc, even with sanitizers
Repros in a small example on windows with msvc using same cpp/hpp files as above link - usually assertion failure in the assertion that this commit removes, sometimes Access Violation, but always crashes one way or another. No sanitizers needed to get a crash with msvc. It's just not easily shareable.