Skip to content

Remove XP (and Server 2003) support from STL #1194

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

Merged
merged 21 commits into from
Sep 25, 2020

Conversation

AlexGuteniev
Copy link
Contributor

Resolves #1192

@StephanTLavavej StephanTLavavej added the performance Must go faster label Aug 14, 2020
@AlexGuteniev
Copy link
Contributor Author

Actually probably need to keep not only _CRTIMP2, but every function visible from header

@AlexGuteniev
Copy link
Contributor Author

I'll only remove functions in anonymous namespace, others cannot be safely removed.

@AlexGuteniev AlexGuteniev marked this pull request as ready for review August 14, 2020 08:31
@AlexGuteniev AlexGuteniev requested a review from a team as a code owner August 14, 2020 08:31
@BillyONeal BillyONeal changed the title Remove XP support from STL Remove XP (and Server 2003) support from STL Aug 25, 2020
# Conflicts:
#	stl/src/awint.hpp
#	stl/src/filesystem.cpp
#	stl/src/winapinls.cpp
#	stl/src/winapisupp.cpp
# Conflicts:
#	stl/src/taskscheduler.cpp
@StephanTLavavej StephanTLavavej self-assigned this Sep 16, 2020
@StephanTLavavej StephanTLavavej mentioned this pull request Sep 23, 2020
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for working on this extensive set of changes! I'll push fixes for ABI compat and a few other things.

@StephanTLavavej
Copy link
Member

Awesome - your PR shrinks the STL's main DLL by 1%! This is an extremely rare gift, thank you! 🎉

Changelog for the commits I pushed:

  • In primitives.hpp, add static_asserts for the size/alignment of stl_MEOW_concrt, and consolidate the comments into a single TRANSITION comment.
  • winapisupp.cpp no longer needs intrin.h; it was previously needed for __crtInitOnceExecuteOnce. (There will be a simple merge conflict with Cleanup headers #1270.)
  • In winapisupp.cpp, SubmitThreadpoolWork and CloseThreadpoolWork are void functions, so we don't need to return them.
  • Fix ABI: awint.hpp was declaring some functions as _CRTIMP2, which expands to __declspec(dllexport). To preserve the exported-ness of these functions, I've simply marked their definitions as _CRTIMP2 (there were a couple of existing occurrences for precedent). This avoids the verbosity of restoring the awint.hpp declarations.
  • Comment all of the now-unused wrapper functions as "TRANSITION, ABI: preserved for binary compatibility", so they can be immediately eliminated in vNext.
  • In filesystem.cpp, previously different preprocessor blocks became possible to fuse; I additionally adjusted their style to match the following preprocessor logic.
  • In winapisupp.cpp, CreateSymbolicLinkW was previously being loaded dynamically, which was being changed to a direct call. However, it is documented as "desktop apps only", and both filesys.cpp and filesystem.cpp guard their calls with _CRT_APP. I'm 98% certain that winapisupp.cpp should do the same thing.

@StephanTLavavej
Copy link
Member

I believe I've successfully ported your stl/CMakeLists.txt changes to stl/msbuild - I pushed a commit.

@StephanTLavavej StephanTLavavej merged commit 554b5d3 into microsoft:master Sep 25, 2020
@StephanTLavavej
Copy link
Member

Thanks again for this wonderful improvement - ripping out unnecessary complicated code is so awesome. 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update OS minimum version and remove XP runtime detection
3 participants