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

Add extern "C++" as a temporary workaround for #include/import coexistence #4154

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Nov 8, 2023

As my <yvals_core.h> comment here explains, extern "C++" allows named modules to work with classically separately compiled code, which we've been doing ever since #3108 implemented the Standard Library Modules. It can also allow named modules to work with classic includes in the same TU, in one specific order.

Originally, I thought that wrapping the STL in extern "C++" would involve changing every header. I discovered that while I needed to audit every header, the necessary changes were far less extensive. I did have to perform several restructurings in other PRs (see below), but after that, we've reached a point where only a few additional changes are necessary. This is because the vast majority of the STL's code is wrapped in the _STD_BEGIN/_STD_END macro pair, which can be easily enhanced to also wrap code in extern "C++" { }. Similarly for our small amount of _STDEXT_BEGIN/_STDEXT_END extensions. For other namespaces, Concurrency is already wrapped in extern "C++" (most of it was applied in ancient times, surprisingly enough, while the rest was added during the initial Standard Library Modules implementation). As for the global namespace, the vast majority of the machinery that the STL declares/defines is extern "C" (which enjoys absolute immunity to module mixing mischief), either because it's UCRT or because that's our modern convention for separately compiled code.

That leaves only a small amount of global namespace code that isn't extern "C", which this PR now wraps in extern "C++" blocks.

The control macro that I'm adding is _USE_EXTERN_CXX_EVERYWHERE_FOR_STL, which defaults to _HAS_CXX20 but can be overridden. (It would be harmless to enable unconditionally, but I figured there was no reason to emit anything for 14/17 modes when it was so easy to avoid.) This will allow us to suppress the workaround when we develop a superior mechanism for avoiding mixing issues and will also allow users to opt-out if they know they are purely using the named module.

The macro pair that I'm adding to the source code is deliberately named _EXTERN_CXX_WORKAROUND/_END_EXTERN_CXX_WORKAROUND because we aren't planning to keep this forever. (Also, I wanted to avoid confusion with the permanent extern "C++" markings on separately compiled declarations.)

Thanks to @philnik777 for suggesting (in Discord) a workaround for dealing with my <ctime> workaround for the UCRT's obnoxious use of static __inline. The <ctime> workaround is a problem here because the Standard Library Module is emitting functions that aren't the same as the ordinary header. Templating the module's functions allows them to coexist with the ordinary functions, due to the overload resolution tiebreaker.

This PR can be reviewed and merged independently, but to take full effect, it needs @cdacamar's compiler fixes MSVC-PR-509344 shipping in VS 2022 17.9 Preview 3. It also needs my #4151 which will be merged soon - note that I've had to include that as a commit here in order to get the tests to pass, which will merge away harmlessly when we commit this. (And it needed my recently merged #4143 #4145 #4146 #4147.) With all of that, this will work, which is further than we've ever gotten before:

C:\Temp>type meow.cpp
#include <__msvc_all_public_headers.hpp>
import std;

int main() {}
C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od /Femeow.exe D:\msvc\binaries\x86chk\modules\std.ixx meow.cpp
std.ixx
meow.cpp
Generating Code...

C:\Temp>

The other order, with import std; before #include, still won't work (it's much more problematic for the compiler). In the medium-term we're looking into developing a "translation mechanism" that will allow arbitrary ordering/interleaving of #include and import to work, with likely superior throughput. In the short term, this PR's unblocking of one specific pattern will allow motivated users to begin exploring the migration of large real-world projects.

I didn't bother with the deprecation/removal typedefs in <ccomplex>, <ciso646>, <cstdalign>, <cstdbool>, and <ctgmath>, as they aren't dragged in by the named module. Also, <__msvc_cxx_stdatomic.hpp> is special and not dragged in by the named module, so I didn't change it either.

I'm adding test coverage to perform basic verification that the classic includes can coexist with the named modules. I'm testing std::vector and std::ranges algorithms as examples of "ordinary citizens" of namespace std, and the Sufficient Additional Overloads (in namespace std for both modules, and in the global namespace for the std.compat module) as the most important examples of things that are being directly marked as extern "C++" here. The test coverage has to be guarded by _MSVC_INTERNAL_TESTING until the compiler fixes ship; I've verified that this is passing internally.

* Didn't bother with these deprecation/removal typedefs, as they aren't dragged in by the named module:
  + `<ccomplex>`
  + `<ciso646>`
  + `<cstdalign>`
  + `<cstdbool>`
  + `<ctgmath>`
* Didn't bother with this special header, as it isn't dragged in by the named module:
  + `<__msvc_cxx_stdatomic.hpp>`
Thanks to philnik777 for the suggestion.
@StephanTLavavej StephanTLavavej added enhancement Something can be improved modules C++23 modules, C++20 header units labels Nov 8, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 8, 2023 16:22
@StephanTLavavej StephanTLavavej self-assigned this Nov 8, 2023
@StephanTLavavej StephanTLavavej removed their assignment Nov 8, 2023
Copy link
Contributor

@cdacamar cdacamar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Finally getting something in place to address this perennial issue 😄.

@jovibor
Copy link
Contributor

jovibor commented Nov 9, 2023

but to take full effect, it needs @cdacamar's compiler fixes MSVC-PR-509344 shipping in VS 2022 17.9 Preview 3

It's really unfortunate that users have to wait literally for months for compiler's important fixes/updates because it's bound to Visual Studio releases, and there is no mechanism of independent compiler upgrade. Would be very nice to have some kind of compiler's "nightly builds". We could test and give feedback a lot quicker.
Sorry for offtopic.

@StephanTLavavej StephanTLavavej self-assigned this Nov 9, 2023
@StephanTLavavej
Copy link
Member Author

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

stl/inc/yvals_core.h Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved modules C++23 modules, C++20 header units
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants