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

Cleanups 7: Unnecessary guard around new_handler #2679

Merged

Conversation

StephanTLavavej
Copy link
Member

This removes the (very strange) guard #if !defined(_INC_NEW) || !defined(_MSC_EXTENSIONS) around the new_handler typedef.

It appears that this guard may have been intended to avoid duplicate typedefs because <new.h> can include <new> (weirdly). However, duplicate typedefs have always been possible - if <new> was included first, !defined(_INC_NEW) would be true, so <new> would emit new_handler. Then if <new.h> is included, with the default of /Ze, #if defined _MSC_EXTENSIONS && !defined _CORECRT_BUILD would be true, so it would also emit new_handler within namespace std. (See C:\Program Files (x86)\Windows Kits\10\Include\10.0.22000.0\ucrt\new.h.) There's no conflict as long as the typedefs are identical.

With this removal, there is no conflict regardless of inclusion order or /Za:

C:\Temp>type meow.cpp
// clang-format off
#if defined(ONLY_NEW_H)
#include <new.h>
#elif defined(ONLY_STD_NEW)
#include <new>
#elif defined(NEW_H_FIRST)
#include <new.h>
#include <new>
#elif defined(STD_NEW_FIRST)
#include <new>
#include <new.h>
#else
#error WOOF
#endif
// clang-format on
#include <type_traits>
static_assert(std::is_same_v<std::new_handler, void (*)()>, "WOOF");
int main() {}
C:\Temp>cl /EHsc /nologo /W4 /DONLY_NEW_H meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /DONLY_STD_NEW meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /DNEW_H_FIRST meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /DSTD_NEW_FIRST meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /Za /DONLY_NEW_H meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /Za /DONLY_STD_NEW meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /Za /DNEW_H_FIRST meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /Za /DSTD_NEW_FIRST meow.cpp
meow.cpp

C:\Temp>

This also removes an unnecessary comment (that essentially repeats the type; it's not like the comment alone would teach people how to use this machinery).

…essary comment.

D:\GitHub\STL\out\build\x64>type meow.cpp
// clang-format off
// clang-format on
static_assert(std::is_same_v<std::new_handler, void (*)()>, "WOOF");
int main() {}

D:\GitHub\STL\out\build\x64>cl /EHsc /nologo /W4 /DONLY_NEW_H meow.cpp
meow.cpp

D:\GitHub\STL\out\build\x64>cl /EHsc /nologo /W4 /DONLY_STD_NEW meow.cpp
meow.cpp

D:\GitHub\STL\out\build\x64>cl /EHsc /nologo /W4 /DNEW_H_FIRST meow.cpp
meow.cpp

D:\GitHub\STL\out\build\x64>cl /EHsc /nologo /W4 /DSTD_NEW_FIRST meow.cpp
meow.cpp

D:\GitHub\STL\out\build\x64>cl /EHsc /nologo /W4 /Za /DONLY_NEW_H meow.cpp
meow.cpp

D:\GitHub\STL\out\build\x64>cl /EHsc /nologo /W4 /Za /DONLY_STD_NEW meow.cpp
meow.cpp

D:\GitHub\STL\out\build\x64>cl /EHsc /nologo /W4 /Za /DNEW_H_FIRST meow.cpp
meow.cpp

D:\GitHub\STL\out\build\x64>cl /EHsc /nologo /W4 /Za /DSTD_NEW_FIRST meow.cpp
meow.cpp

D:\GitHub\STL\out\build\x64>
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Apr 27, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner April 27, 2022 22:33
@StephanTLavavej StephanTLavavej added this to Final Review in Code Reviews Apr 27, 2022
@strega-nil-ms strega-nil-ms self-assigned this Apr 27, 2022
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Apr 28, 2022
@CaseyCarter CaseyCarter removed their assignment Apr 28, 2022
@strega-nil-ms strega-nil-ms removed their assignment Apr 28, 2022
@StephanTLavavej StephanTLavavej self-assigned this Apr 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.

@StephanTLavavej StephanTLavavej merged commit 4b3d984 into microsoft:main May 1, 2022
Code Reviews automation moved this from Ready To Merge to Done May 1, 2022
@StephanTLavavej StephanTLavavej deleted the cleanups-7-new_handler branch May 1, 2022 10:46
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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants