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

<typeinfo>: Error with import std; and typeid(T).name() #3677

Merged
merged 3 commits into from
May 5, 2023

Conversation

StephanTLavavej
Copy link
Member

@cdacamar explained that in this case, the MSVC compiler looks for the True Name of type_info in the global namespace, so we need to export it as a special exception.

I'm adding test coverage for this scenario. The using namespace std; in this test doesn't actually affect the repro, so I didn't bother to remove it.

Fixes DevCom-10349749 and internal VSO-1810086 / AB#1810086 .

@StephanTLavavej StephanTLavavej added bug Something isn't working modules C++23 modules, C++20 header units labels Apr 28, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner April 28, 2023 21:49
@github-actions github-actions bot added this to Initial Review in Code Reviews Apr 28, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Apr 28, 2023
@strega-nil-ms
Copy link
Contributor

Is this conforming? I'm okay with "no, but it needs to not be conforming for our implementation", but I'm wondering if it'd be possible to make the compiler search in std:: here, rather than continuing the non-conformance.

@StephanTLavavej
Copy link
Member Author

@cdacamar said on Discord:

I suppose I could change that, but I don't want to introduce a compatibility problem 😦

The Standardese that I wrote, N4944 [std.modules]/6, says:

Recommended practice: Implementations should avoid exporting any other declarations from the C++ library modules.

IMO it's slightly nonconforming to emit ::type_info because it could clash with user code, but the same is also true for classic <type_info> (it's not a reserved name, nor is it one of the C names that is in a superposition between global and std).

In practice I think it's easiest to just make this special exception and not worry about it further.

@CaseyCarter
Copy link
Member

I suppose I can live with this as extending our pre-existing non-conformance into the Modules Universe.

@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews May 1, 2023
@StephanTLavavej StephanTLavavej self-assigned this May 4, 2023
@StephanTLavavej
Copy link
Member Author

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

@StephanTLavavej StephanTLavavej merged commit 1784bd4 into microsoft:main May 5, 2023
Code Reviews automation moved this from Ready To Merge to Done May 5, 2023
@StephanTLavavej StephanTLavavej deleted the import-typeid branch May 5, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working modules C++23 modules, C++20 header units
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants