Cleanup product code uses of feature-test macros #3053
Merged
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
We should try to minimize the number of feature-test macros used to guard conditionally-compiled product code, so that readers aren't required to memorize the content of SD-6 in order to comprehend the STL. Consequently, feature-test macros should only be used in product code when their values are not implied by language mode or a conjunction of language mode and some other feature-test macro. We should instead test `_HAS_CXXYY && defined(__other_feature_macro)` directly. Feature-test macros whose values vary according to compiler flags other than language modes, or for features that aren't yet implemented by all compilers should be tested directly (although latter will eventually be replace by langage mode tests when the minimum required versions of all supported compilers implement the feature). Some consequences of these rules: * `_HAS_CXX20 && defined(__cpp_lib_concepts)` is redundant since `defined(__cpp_lib_concepts)` implies `_HAS_CXX20`. * `_HAS_CXX20` implies `__cpp_constexpr_dynamic_alloc` now that all supported C++20 front ends implement that feature * `defined(__cpp_lib_concepts)` implies `defined(__cpp_lib_format)` * `_HAS_CXX23 && defined(__cpp_lib_concepts)` implies `defined(__cpp_expected)` and `defined(__cpp_lib_containers_ranges)` [Note that we don't annotate plain `defined(__cpp_lib_concepts)` with `// TRANSITION, microsoftGH-395` in product because microsoftGH-395 clearly indicates that all such tests should be replaced and we don't want to churn just to add the annotation.]
StephanTLavavej
requested changes
Aug 24, 2022
* remove redundant nested `_HAS_CXX23`s * remove `// TRANSITION, microsoftGH-395` from `#ifn?def __cpp_lib_concepts` for consistency * don't be lazy and use `// TRANSITION, microsoftGH-395` as an `#endif` comment instead of the actual test condition
StephanTLavavej
approved these changes
Aug 25, 2022
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
Thanks for increasing codebase consistency! 🧹 ✨ 💯 |
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.
We should try to minimize the number of feature-test macros used to guard conditionally-compiled product code, so that readers aren't required to memorize the content of SD-6 in order to comprehend the STL. Consequently, feature-test macros should only be used in product code when their values are not implied by language mode or a conjunction of language mode and some other feature-test macro. We should instead test
_HAS_CXXYY && defined(__other_feature_macro)directly. Feature-test macros whose values vary according to compiler flags other than language modes, or for features that aren't yet implemented by all compilers should be tested directly (although latter will eventually be replace by langage mode tests when the minimum required versions of all supported compilers implement the feature).Some consequences of these rules:
_HAS_CXX20 && defined(__cpp_lib_concepts)is redundant sincedefined(__cpp_lib_concepts)implies_HAS_CXX20._HAS_CXX20implies__cpp_constexpr_dynamic_allocnow that all supported C++20 front ends implement that featuredefined(__cpp_lib_concepts)impliesdefined(__cpp_lib_format)_HAS_CXX23 && defined(__cpp_lib_concepts)impliesdefined(__cpp_expected)anddefined(__cpp_lib_containers_ranges)Note that we don't annotate plain
defined(__cpp_lib_concepts)with// TRANSITION, GH-395in product because GH-395 clearly indicates that all such tests should be replaced and we don't want to churn just to add the annotation. I could be convinced to apply such annotations and remove the special case if people really want to review another few hundred mechanical changes, but for now I have instead stripped such transition comments from the few places they did appear.