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

Move machinery from inc/ymath.h to src/xmath.hpp #2828

Merged
merged 2 commits into from
Jul 1, 2022

Conversation

StephanTLavavej
Copy link
Member

While looking at the STL's separately compiled machinery for modules, I noticed that inc/ymath.h was declaring a bunch of stuff that isn't needed by any other header in inc. We can move this to src/xmath.hpp (which is being included by everything in src that needs it) without disrupting users or affecting ABI. (That is, the exported machinery in src will still be exported; there's no reason to declare it as imported in inc if we aren't going to use it there.)

This is theoretically an extremely slight throughput improvement (fewer things being declared), but realistically it just makes the codebase easier to understand - fewer spurious connections between inc and src.


I'm changing one thing while moving this from inc to src: changing _CRTIMP2_PURE_IMPORT to _CRTIMP2_PURE. Within src, they are exactly identical (and we don't use _CRTIMP2_PURE_IMPORT except for _Yarn - that one is trying to match the header stylistically, although it could be changed).

Click to expand macro definitions:

STL/stl/inc/yvals.h

Lines 281 to 297 in ad80eb7

#ifndef _CRTIMP2_IMPORT
#if defined(CRTDLL2) && defined(_CRTBLD)
#define _CRTIMP2_IMPORT __declspec(dllexport)
#elif defined(_DLL) && !defined(_STATIC_CPPLIB)
#define _CRTIMP2_IMPORT __declspec(dllimport)
#else
#define _CRTIMP2_IMPORT
#endif
#endif // _CRTIMP2_IMPORT
#ifndef _CRTIMP2_PURE_IMPORT
#ifdef _M_CEE_PURE
#define _CRTIMP2_PURE_IMPORT
#else
#define _CRTIMP2_PURE_IMPORT _CRTIMP2_IMPORT
#endif
#endif // _CRTIMP2_PURE_IMPORT

<crtdefs.h>:

#ifndef _CRTIMP2
    #if defined CRTDLL2 && defined _CRTBLD
        #define _CRTIMP2 __declspec(dllexport)
    #else
        #define _CRTIMP2
    #endif
#endif

STL/stl/inc/yvals.h

Lines 265 to 271 in ad80eb7

#ifndef _CRTIMP2_PURE
#ifdef _M_CEE_PURE
#define _CRTIMP2_PURE
#else
#define _CRTIMP2_PURE _CRTIMP2
#endif
#endif // _CRTIMP2_PURE

The only difference between _CRTIMP2_PURE_IMPORT and _CRTIMP2_PURE is when building user code using the STL as a DLL (/MD and /MDd), in which case the former expands to __declspec(dllimport). That's impossible for stl/src, and we don't need to match any other declarations, so we should use plain _CRTIMP2_PURE like everything else.


We have export validation in the internal build, but I also verified this manually.

First, notice that _Feraise wasn't marked _CRTIMP2_PURE, so it wasn't being imported/exported at all! With main, it doesn't appear in the export surface:

D:\GitHub\STL\out\build\x64\out\bin\amd64>dumpbin /exports msvcp140_oss.dll | grep -P _Feraise

D:\GitHub\STL\out\build\x64\out\bin\amd64>

With this PR, everything else is still being exported:

D:\GitHub\STL\out\build\x64\out\bin\amd64>dumpbin /exports msvcp140_oss.dll | grep -P "\b(_Dtest|_FDtest|_Denorm|_Hugeval|_Inf|_Nan|_Snan|_FDenorm|_FInf|_FNan|_FSnan|_LDenorm|_LInf|_LNan|_LSnan)\b"
       1371  55A 00082198 _Denorm = _Denorm
       1372  55B 00046670 _Dtest = _Dtest
       1376  55F 00082108 _FDenorm = _FDenorm
       1377  560 00046EB0 _FDtest = _FDtest
       1379  562 000820E8 _FInf = _FInf
       1380  563 000820F8 _FNan = _FNan
       1382  565 00082118 _FSnan = _FSnan
       1391  56E 00082178 _Hugeval = _Hugeval
       1392  56F 000821A8 _Inf = _Inf
       1394  571 00082148 _LDenorm = _LDenorm
       1397  574 00082128 _LInf = _LInf
       1398  575 00082138 _LNan = _LNan
       1400  577 00082158 _LSnan = _LSnan
       1423  58E 00082168 _Nan = _Nan
       1433  598 00082188 _Snan = _Snan

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jun 28, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner June 28, 2022 05:39
@StephanTLavavej StephanTLavavej self-assigned this Jun 30, 2022
@StephanTLavavej
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants