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.

Sign up for GitHub

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

Jump to bottom

Hasta la Vista #2317

Merged
merged 12 commits into from
Jul 1, 2022
Merged

Hasta la Vista #2317

merged 12 commits into from
Jul 1, 2022

Conversation

AlexGuteniev
Copy link
Contributor

Resolves #2286

Wanted to combine with #2309, but apparently constexpr mutex
will have long way to go if ever succeeds

@AlexGuteniev
Fully drop Vista
505ec4c 
Resolves microsoft#2286

Wanted to combine with microsoft#2309, but apparently constexpr mutex
will have long way to go if ever succeeds
@AlexGuteniev AlexGuteniev requested a review from a team as a code owner November 2, 2021 19:12
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Nov 2, 2021
@StephanTLavavej StephanTLavavej self-assigned this Nov 3, 2021
@StephanTLavavej StephanTLavavej added performance Must go faster and removed enhancement Something can be improved labels Nov 3, 2021
@StephanTLavavej StephanTLavavej added the affects redist Results in changes to separately compiled bits label Dec 15, 2021
AlexGuteniev and others added 9 commits December 18, 2021 13:30
@AlexGuteniev
Merge remote-tracking branch 'upstream/main' into hasta_la_vista
88de427
@AlexGuteniev
Merge remote-tracking branch 'upstream/main' into hasta_la_vista
8d62429
@AlexGuteniev
Merge remote-tracking branch 'upstream/main' into hasta_la_vista
ef4f386
@AlexGuteniev
Merge remote-tracking branch 'upstream/main' into hasta_la_vista
0dfc544
@StephanTLavavej
Merge branch 'main' into hasta_la_vista
c14b532
@StephanTLavavej
Increase _STL_WIN32_WINNT default to Win7.
811b6b4 
Note that the DLL build still overrides this to XP.
@StephanTLavavej
Remove __stl_sync_api_impl_mode.
b0327b9 
This is now dead - its value is never read. We need to keep
`__set_stl_sync_api_mode()` as a no-op for the DLL's export surface,
appropriately commented. (Long ago, this function was intended to allow
testing of downlevel OS fallback codepaths on modern OS machines; it
had no other purpose during normal operation.)

Note that `__stl_sync_api_impl_mode` wasn't mentioned outside of
`stl/src`, so we don't have to worry about previously compiled user
code looking for it.
@StephanTLavavej
Temporary static_asserts before removing code.
1c88e7d 
Passed for x86, x64, ARM, ARM64.
@StephanTLavavej
Remove stl_critical_section_vista, stl_condition_variable_vista.
b08c3fc
StephanTLavavej
StephanTLavavej approved these changes May 28, 2022
View reviewed changes
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! I'll push changes to eradicate even more Vista machinery.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented May 28, 2022 •
edited
Loading

I pushed the following additional changes:

  • Increase _STL_WIN32_WINNT default to Win7.
    • Note that the DLL build still overrides this to XP.
  • Remove __stl_sync_api_impl_mode.
    • This is now dead - its value is never read. We need to keep __set_stl_sync_api_mode() as a no-op for the DLL's export surface, appropriately commented. (Long ago, this function was intended to allow testing of downlevel OS fallback codepaths on modern OS machines; it had no other purpose during normal operation.)
    • Note that __stl_sync_api_impl_mode wasn't mentioned outside of
      stl/src, so we don't have to worry about previously compiled user
      code looking for it.
  • Remove stl_critical_section_vista, stl_condition_variable_vista.
    • Like Remove unused ConcRT wrappers #1325, we can replace those classes with their size/alignment values. I used temporary static_asserts (passing for x86, x64, ARM, and ARM64) to verify the values.
    • We can eliminate the __max() usage because the concrt sizes are greater than (or equal to) the vista sizes, which in turn are greater than (or equal to) the win7 sizes.
    • The alignments are all identical; I extracted them for clarity. (I felt that using alignof(win7) was clearer than alignof(void*).)

I believe that it makes sense to do this simultaneously, but these commits could be done as a followup PR if anyone is concerned about the complexity.

@StephanTLavavej StephanTLavavej removed their assignment May 28, 2022
@StephanTLavavej StephanTLavavej added the blocked Something is preventing work on this label May 28, 2022
@StephanTLavavej
Copy link
Member

Labeling as blocked only because we need to wait to merge this until we're certain that changes are flowing into 17.4 Preview 2 specifically (right now it might be Preview 1).

@StephanTLavavej
Copy link
Member

@barcharcraz mentioned that we've likely already dropped Vista support by merging #2654 previously, which is yet another reason that this PR is okay to merge.

@StephanTLavavej StephanTLavavej removed the blocked Something is preventing work on this label Jun 29, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jun 30, 2022
@StephanTLavavej
Copy link
Member

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

@CaseyCarter
Copy link
Contributor

image

@StephanTLavavej StephanTLavavej merged commit 1f37fe7 into microsoft:main Jul 1, 2022
@StephanTLavavej
Copy link
Member

Thank you for improving library performance by eradicating this ancient machinery! I never thought I'd see the day! 😻 🐴 🦄

@AlexGuteniev AlexGuteniev deleted the hasta_la_vista branch July 1, 2022 04:05
@StephanTLavavej StephanTLavavej mentioned this pull request Jul 2, 2022
Merged
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
@AlexGuteniev @StephanTLavavej @fsb4000
Hasta la Vista (microsoft#2317)
6a4a7dd 
Co-authored-by: Stephan T. Lavavej <[email protected]>
@frederick-vs-ja frederick-vs-ja mentioned this pull request Mar 3, 2023
Open
@cpplearner cpplearner mentioned this pull request Jun 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@barcharcraz barcharcraz barcharcraz approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
affects redist Results in changes to separately compiled bits performance Must go faster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop support for targeting Windows Vista / Server 2008
4 participants
@AlexGuteniev @StephanTLavavej @CaseyCarter @barcharcraz