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

Various cleanups #3935

Merged
merged 23 commits into from Aug 11, 2023
Merged

Various cleanups #3935

merged 23 commits into from Aug 11, 2023

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Aug 8, 2023

This PR is admittedly somewhat hypocritical as I've mentioned that maintainer capacity is sharply limited and cleanups take time away from features and bugfixes (including libcxx skip reduction which I'd like to see). That said, while reviewing PRs, I notice a lot of things that aren't worth resetting testing of those PRs, but collectively add up.

We strongly discourage "grab bag" PRs, so I've separated out #3934 as a fairly large semi-mechanical test change. The remaining changes here are numerous, but individually small and safe, so it's easier to review (and merge) one PR with well-structured commits instead of 20 ultra-tiny PRs. (The problem with "grab bag" PRs is when large changes are mixed with other changes, or when major semantic changes are mixed with cleanups. The impact of each change here is very small.)

Nevertheless, I can decompose this into smaller PRs if desired (that's the great thing about having fine-grained commits).

The commits:

  • Guard _Xtime_diff_to_millis2 with #ifdef _CRTBLD.
    • This isn't needed by stl/inc headers, so we don't need to emit this declaration for users. (This doesn't affect the export surface since we continue to declare it for stl/src and the declaration matches the definition. We shouldn't have exported it at all, which we can fix during vNext - we don't really need an explicit comment saying so, since the presence of #ifdef _CRTBLD is sufficient.)
  • Guard _Execute_once, _Execute_once_fp_t with #ifdef _CRTBLD.
    • Ditto.
  • Style: Group __msvc_sanitizer_annotate_container.hpp with other unconditionally included headers.
    • This was added at the bottom during maintenance, but there's (fortunately) no ordering requirement. We should let clang-format sort this normally.
  • Formatting: Drop a spurious newline in a basic_istream constructor.
  • Drop _CONSTEVAL, _Cx_exp2() can be plain constexpr.
    • The only usage of _Cx_exp2() is initializing static constexpr _Cy_t _Cy and static constexpr _Ty _Scale1, so it's always being evaluated at compile-time.
  • Style: Change unnecessarily const _Auto_id_tag unnamed parameters.
    • Most were already non-const.
  • Add explicit _Auto_id_tag() = default; to defend against mistakes.
    • The Standard now does this conventionally (it defends against {}), so we should do this for our own tags.
  • Fix citation to nonexistent N5687.
  • Update links: docs.microsoft.com => learn.microsoft.com
    • This skips a domain redirection. This also updates a support.microsoft.com URL in the README that's being redirected. Finally, this adds specific links to StlCompareStringA.cpp, StlCompareStringW.cpp, StlLCMapStringA.cpp, StlLCMapStringW.cpp. Intentionally not changing SECURITY.md.
  • Add space to comment.
  • Style: typename => class in product code.
  • Fuse adjacent Standard modes.
  • Simplify away _THREAD_CHECK, _THREAD_CHECKX.
    • We never defined _THREAD_CHECK in the internal or GitHub builds, so this code simply collapses to #ifdef _DEBUG.
  • Drop polluting using-declarations within stdext.
    • We don't need to qualify size_t at all. Intentionally not attempting to make <hash_map> and <hash_set> less polluting.
    • This is the only potentially source-breaking change, but it seems unlikely that users would rely on this pollution, and fixing affected code is trivial.
  • Mark _Big_uint128::operator< as _NODISCARD.
  • Mark uncaught_exception() and uncaught_exceptions() as _NODISCARD.
    • I've updated the definitions to repeat the attributes, and to drop useless comments (we know from the Standard what these functions are supposed to do). Note that _EXPORT_STD extern "C++" is intentionally not repeated as the separately compiled STL is always built clasically.
  • Simplify CMakeLists.txt: MATCHES "\^(\w+)\$" => STREQUAL "$1"
  • Simple test cleanup: After STATIC_ASSERT(CanViewElements<Rng&>), assume it's true.
    • We caught most occurrences of this during code review, but missed this one. It wasn't being deliberately weird.
  • <random>: Decompose complicated conditional expression.
    • This is more lines, but easier to follow. (In modern code, we prefer to avoid mixing side-effects with other expressions when they aren't highly idiomatic, and we usually prefer to avoid conditional expressions when there isn't a strong motivation for them.) Note that the result of the compound assignment is the left operand. The if-return-else-return is deliberate.
  • Simplify _Cnd_internal_imp_t size/align constants.
    • The stl_condition_variable_max_MEOW constants in primitives.hpp were exactly identical to the _Cnd_internal_imp_MEOW constants in xthreads.h, because we use _Aligned_storage with no other data members. This unifies them, keeping the xthreads.h names but the simpler primitives.hpp definition technique and clearer comments. Finally, this makes the static_asserts in cond.cpp tautological, so this removes them.

The only usage of `_Cx_exp2()` is initializing `static constexpr _Cy_t _Cy` and `static constexpr _Ty _Scale1`, so it's always being evaluated at compile-time.
This skips a domain redirection.

This also updates a `support.microsoft.com` URL in the README that's being redirected.

Finally, this adds specific links to `StlCompareStringA.cpp`, `StlCompareStringW.cpp`, `StlLCMapStringA.cpp`, `StlLCMapStringW.cpp`.

Intentionally not changing `SECURITY.md`.
We **never** defined `_THREAD_CHECK` in the internal or GitHub builds,
so this code simply collapses to `#ifdef _DEBUG`.
We don't need to qualify `size_t` at all.

Intentionally not attempting to make `<hash_map>` and `<hash_set>` less polluting.
It appears that we don't usually mark the definitions (but perhaps we should).
This is more lines, but easier to follow.

Note that the result of the compound assignment is the left operand.

The if-return-else-return is deliberate.
The `stl_condition_variable_max_MEOW` constants in `primitives.hpp` were exactly identical to the `_Cnd_internal_imp_MEOW` constants in `xthreads.h`, because we use `_Aligned_storage` with no other data members.

This unifies them, keeping the `xthreads.h` names but the simpler `primitives.hpp` definition technique and clearer comments.

Finally, this makes the `static_assert`s in `cond.cpp` tautological, so this removes them.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Aug 8, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 8, 2023 01:22
@github-actions github-actions bot added this to Initial Review in Code Reviews Aug 8, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Aug 8, 2023
stl/src/mutex.cpp Outdated Show resolved Hide resolved
stl/src/StlCompareStringA.cpp Outdated Show resolved Hide resolved
stl/inc/xcall_once.h Show resolved Hide resolved
stl/inc/exception Show resolved Hide resolved
stl/inc/xthreads.h Show resolved Hide resolved
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Aug 9, 2023
@CaseyCarter CaseyCarter removed their assignment Aug 9, 2023
@StephanTLavavej
Copy link
Member Author

@CaseyCarter I've pushed a commit to address one of your comments.

@StephanTLavavej StephanTLavavej self-assigned this Aug 10, 2023
@StephanTLavavej
Copy link
Member Author

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

@StephanTLavavej
Copy link
Member Author

I've pushed a merge with main to resolve merge conflicts where #3900 updated preprocessor comments.

  • Resolved in stl/inc/xthreads.h by accepting this PR's new definitions of _Cnd_internal_imp_MEOW with well-commented preprocessor logic.
  • Resolved in stl/src/mutex.cpp by upgrading this PR's preprocessor comments to defined(_DEBUG) arrow comments.

@StephanTLavavej StephanTLavavej merged commit 036ea59 into microsoft:main Aug 11, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Aug 11, 2023
@StephanTLavavej StephanTLavavej deleted the stl-cleanups branch August 11, 2023 00:44
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