Add extern "C++" as a temporary workaround for #include/import coexistence#4154
Merged
StephanTLavavej merged 4 commits intomicrosoft:mainfrom Nov 10, 2023
Merged
Conversation
* 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.
cdacamar
approved these changes
Nov 8, 2023
Contributor
cdacamar
left a comment
There was a problem hiding this comment.
Nice work! Finally getting something in place to address this perennial issue 😄.
Contributor
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. |
Member
Author
|
I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
CaseyCarter
approved these changes
Nov 10, 2023
Merged
This was referenced Apr 20, 2024
This was referenced May 10, 2024
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
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.
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_ENDmacro pair, which can be easily enhanced to also wrap code inextern "C++" {}. Similarly for our small amount of_STDEXT_BEGIN/_STDEXT_ENDextensions. For other namespaces,Concurrencyis already wrapped inextern "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 isextern "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 inextern "C++"blocks.The control macro that I'm adding is
_USE_EXTERN_CXX_EVERYWHERE_FOR_STL, which defaults to_HAS_CXX20but 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_WORKAROUNDbecause we aren't planning to keep this forever. (Also, I wanted to avoid confusion with the permanentextern "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 ofstatic __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:
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#includeandimportto 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::vectorandstd::rangesalgorithms as examples of "ordinary citizens" ofnamespace std, and the Sufficient Additional Overloads (innamespace stdfor both modules, and in the global namespace for thestd.compatmodule) as the most important examples of things that are being directly marked asextern "C++"here. The test coverage has to be guarded by_MSVC_INTERNAL_TESTINGuntil the compiler fixes ship; I've verified that this is passing internally.