-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Split/fuse xlocinfo.h
to fix unintentional export in format.cpp
#1927
Conversation
In xwctomb.cpp, need to reorder functions. Also, <cwchar> provides wmemcmp(), not <cstring>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - thanks for picking this up.
Before this change we had both |
Correct - with header guards differing by one crucial underscore 🙀 Lines 6 to 8 in cb37189
Lines 6 to 8 in cb37189
No, it's eventually included by Standard headers; one path is: Line 16 in cb37189
Line 13 in cb37189
Line 11 in cb37189
At this time, I believe everything in |
Thanks for fixing this bug in which some STL developer - who we won't name and shame here - added an unintentional export to the import library. (Hint: it was me.) |
This affects separately compiled code, but shouldn't modify the DLL's export surface or break ABI in any other way. The second degree of caution is warranted.
In certain modules scenarios (both Standard Library Header Units, and experimental C++23 modules), compiling EXEs dragging in
format.obj
would display "Creating librarymeow.lib
and objectmeow.exp
", and the resulting EXE would export_Init_locks::operator=
. This was definitely unintentional.The problem was that
format.cpp
, injected into the import lib, was includingxlocinfo.h
. I am not 100% certain of the exact chain of events, but it appears that in certain cases, this would drag in_Init_locks
, which we don't need or want here.Separately,
xlocinfo.h
was problematic for deduplicated Standard Library Header Units, because building both<xlocinfo>
and<xlocinfo.h>
wants to createxlocinfo.obj
by default. No other STL headers have the same name like this.The fix to both of these problems is to split
xlocinfo.h
's contents. I'm adding a minimal header__msvc_xlocinfo_types.hpp
(qualifying as a "core" header), which defines just the_Collvec
,_Ctypevec
, and_Cvtvec
structs, which have no further dependencies. The rest ofxlocinfo.h
is then fused intoxlocinfo
.format.cpp
needs only_Cvtvec
, but by adding the two other structs into the mini-header, we can change all of thesrc
files to include just that mini-header (they don't need the formerxlocinfo.h
's function declarations to provide their function definitions).No DLL-exported functions are being modified here. This modifies
format.obj
in the import lib, which is effectively statically linked, so there's no compatibility concern.Additional notes:
src
files were already includingyvals.h
(which provides our export macros), sometimes viaawint.hpp
, butxmbtowc.cpp
andxwctomb.cpp
need to explicitly include it.xwcscoll.cpp
was including<cstring>
forwmemcmp()
but that's incorrect - it needs to include<cwchar>
. (The VSO-661721 thing about needing to include<cstdio>
before<cwchar>
applies to header files only, not source files like this one.)xwctomb.cpp
needs to define_Getcvt()
and_Wcrtomb()
before calling them, so their definitions are being reordered here. There are no other changes (and these aren't something like the initial declarations of virtual functions where the order matters).