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

<any>: Skip the contents when static RTTI is disabled #3115

Merged
merged 7 commits into from
Sep 22, 2022

Conversation

frederick-vs-ja
Copy link
Contributor

This PR is making #include <any> effectless rather than erroneous when static RTTI is disabled.

Fixes #3114.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner September 20, 2022 02:14
@CaseyCarter CaseyCarter added the bug Something isn't working label Sep 20, 2022
stl/inc/any Outdated Show resolved Hide resolved
stl/inc/any Outdated Show resolved Hide resolved
stl/inc/any Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
Co-authored-by: Casey Carter <Casey@Carter.net>
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation Sep 20, 2022
@CaseyCarter CaseyCarter moved this from Initial Review to Final Review in Code Reviews Sep 20, 2022
@strega-nil-ms
Copy link
Contributor

Do we want to additionally not include <any> in std.ixx when _HAS_STATIC_RTTI=0? It seems like we shouldn't be printing an error here.

@StephanTLavavej
Copy link
Member

@strega-nil-ms Agreed - the intention is for std.ixx to respect all of our various modes. I can validate and push changes.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 21, 2022

Ok, I've pushed the following changes (FYI @CaseyCarter @strega-nil-ms):

  • std.ixx: Guard <any> with _HAS_STATIC_RTTI.
    • <algorithm> is always included first, so <yvals_core.h> is available.
  • Add /D_HAS_STATIC_RTTI=0 coverage to P2465R3_standard_library_modules.
    • Also reduce the /analyze coverage to one configuration. (It found no issues when originally added, so we don't need to spend an entire crosslist on it.) Combined, this reduces the number of configurations from 8 to 6.
  • Guard __cpp_lib_any with _HAS_STATIC_RTTI, add test coverage.
    • We should have done this earlier (as we do for headers where /clr:pure is blocked, etc.) but now is also a good time.
  • VSO_0000000_has_static_rtti: Comment the apparently unused include.

Thanks @frederick-vs-ja for working on this - if we're going to have this not-exactly-encouraged-but-still-supported mode, it should behave reasonably, and I think your change makes the story coherent. 😻

✅ Note to self: modules changes verified.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Sep 21, 2022
@StephanTLavavej StephanTLavavej self-assigned this Sep 22, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 3323648 into microsoft:main Sep 22, 2022
Code Reviews automation moved this from Ready To Merge to Done Sep 22, 2022
@StephanTLavavej
Copy link
Member

Thanks for fixing the first bug after Standard Library Modules was merged! 🐞 ✅ 😻

@frederick-vs-ja frederick-vs-ja deleted the no-rtti-no-any branch September 22, 2022 23:24
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Oct 6, 2022
Co-authored-by: Casey Carter <Casey@Carter.net>
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

std.ixx: Defining _HAS_STATIC_RTTI=0 and importing std fails to compile due to any.
4 participants