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 #4230

Merged
merged 10 commits into from
Dec 7, 2023
Merged

Conversation

StephanTLavavej
Copy link
Member

Usual disclaimer about how I'm being a bit of a bad kitty 😼 because "grab bag PRs" can be challenging to review and risky to merge. This PR is structured into fine-grained commits for easier reviewing and restricts itself to very low-risk changes (I have separate PRs for fixing libcxx failures).

  • <optional>: Drop unused requires-expression parameters that appeared only in operator<=.
  • <yvals.h>: _THROW should expand with parens.
  • <condition_variable> and <thread> should immediately reject /clr:pure.
    • All other headers (e.g. <mutex>) follow this pattern.
  • Comment that _Stoll, _Stoul, _Stoull are preserved for bincompat. Nobody declares or calls them, including via the CNAME macro.
    • Also, don't defend _Stoll's definition against macros - this is stl/src where we have complete control.
  • <mdspan>: operator[] overloads can attach their open braces.
    • clang-format exhibits "sticky" behavior here - it's happy either way.
  • <yvals_core.h>: Improve the _USE_EXTERN_CXX_EVERYWHERE_FOR_STL comment.
  • Drop low-value file comments and outdated "for ConcRT" comment.
    • This was the only remaining mention of ConcRT outside of ABI-preserved code. We don't need to preserve the "condition variable implementation" part for _Cnd_internal_imp_t as the type name is sufficiently clear.
    • This doesn't drop all file comments, just the ones that I thought were especially unnecessary (due to the filenames and file contents).
  • Remove more unnecessary file comments.
  • Drop unnecessary/inconsistent newlines after open braces.
    • This doesn't attempt to drop all newlines after open braces - many of them are arguably providing clarity.
  • Drop unnecessary/inconsistent newlines before #else.
    • Again, this doesn't attempt to drop all such newlines, many of which are providing clarity. The occurrences I'm changing, all involving _ITERATOR_DEBUG_LEVEL or _M_CEE_PURE, looked imbalanced and were ancient relics of codebase processing.

Pre-existing since the beginning of time, noted in the review for GH 4119.

clang-format produces `throw(__VA_ARGS__)` with no space, which is annoying but not worth suppressing.
…pure.

All other headers follow this pattern.
Nobody declares or calls them, including via the CNAME macro.

Also, don't defend `_Stoll`'s definition against macros - this is stl/src where we have complete control.
clang-format exhibits "sticky" behavior here.
Noted during the review of GH 4154, took that suggestion exactly.
This doesn't drop all file comments, just the ones that I thought were especially unnecessary.
Every single one of these was `// MEOW function` where `MEOW` appears below (without any macroization).
This doesn't attempt to drop all newlines after open braces - many of them are arguably providing clarity.
Again, this doesn't attempt to drop all such newlines, many of which are providing clarity.

These occurrences, all involving `_ITERATOR_DEBUG_LEVEL` or `_M_CEE_PURE`, looked imbalanced and were ancient relics of codebase processing.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Dec 2, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner December 2, 2023 01:45
@@ -452,7 +451,7 @@ class _CRTIMP2_PURE_IMPORT _EmptyLockit { // empty lock class used for bin compa
#define _CATCH_END }

#define _RERAISE throw
#define _THROW(...) throw __VA_ARGS__
#define _THROW(...) throw(__VA_ARGS__)
Copy link
Member

Choose a reason for hiding this comment

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

throw is not a function (not worth fighting with clang-format if it really wants it this way).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I called this out in my PR description - it's clang-format's doing, and it'll remove any attempt to add a space here.

@CaseyCarter CaseyCarter removed their assignment Dec 6, 2023
@StephanTLavavej StephanTLavavej self-assigned this Dec 7, 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 StephanTLavavej merged commit 4c110bf into microsoft:main Dec 7, 2023
37 checks passed
@StephanTLavavej StephanTLavavej deleted the various-cleanups branch December 7, 2023 16:41
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