KEMBAR78
Casey's accumulated miscellaneous changes by CaseyCarter · Pull Request #4900 · microsoft/STL · GitHub
Skip to content

Conversation

@CaseyCarter
Copy link
Contributor

@CaseyCarter CaseyCarter commented Aug 20, 2024

A grab bag of minor changes broken down for quick-and-easy commit-wise review:

  • <ranges>: simplify zip_view::_Size_closure by making it a static constexpr data member instead of a function
  • Use conventional LLVM issue linking style
  • Simplify _Tuple_like metaprogramming: implements _Tuple_like_impl as a variable template with four partial specializations instead of as a disjunction of four other variable template specializations. Consequently, each evaluation of _Tuple_like causes the compiler to memoize one constexpr variable instead of five.
  • Don't return a decayed boolean-testable since the exposition-only concept doesn't allow for construction of an object of decayed type from a model of boolean-testable. Note that the changes in erase_if(forward_list, pred) and erase_if(list, pred) are the proposed resolution of LWG-4135, which I feel is a no-brainer and can be implemented without comments. I audited for other occurrences after noticing we do the same thing in forward_list::remove and list::remove.
    The fold expressions in <ranges> are a weird case: they are only problematic when the pack has one element. For zero or more than one elements, the fold expression has type bool, but with exactly one element it's a boolean-testable that the lambda implicitly decays on return. I think it's a bit more elegant to fix these by adding || false to the fold expressions to make them binary than to explicitly specify the return type of the lambda.
  • Don't return in cartesian_product_view::_Iterator::iter_swap. We can return an expression with void type in a function with return type void, but we typically don't do so when the types are concrete.

I could be convinced that the boolean-testable change deserves to be in a separate PR with some test coverage.

CaseyCarter and others added 5 commits August 20, 2024 12:04
... by making it a `static constexpr` data member instead of a function
Implements `_Tuple_like_impl` as a variable template with four partial specializations instead of as a disjunction of four other variable template specializations. Consequently, each evaluation of `_Tuple_like` causes the compiler to memoize one constexpr variable instead of five.
... since the exposition-only concept doesn't allow for construction of an object of decayed type from a model of *boolean-testable*. Note that the changes in `erase_if(forward_list, pred)` and `erase_if(list, pred)` are the proposed resolution of LWG-4135, which I feel is a no-brainer and can be implemented without comments. I audited for other occurrences after noticing we do the same thing in `forward_list::remove` and `list::remove`.

The fold expressions in `<ranges>` are a weird case: they are only problematic when the pack has one element. For zero or more than one elements, the fold expression has type `bool`, but with exactly one element it's a *boolean-testable* that the lambda implicitly decays on return. I think it's a bit more elegant to fix these by adding `|| false` to the fold expressions to make them binary than to explicitly specify the return type of the lambda.
We _can_ `return` an expression with `void` type in a function with return type `void`, but we typically don't do so when the types are concrete.
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Aug 20, 2024
@CaseyCarter CaseyCarter requested a review from a team as a code owner August 20, 2024 19:11
@StephanTLavavej StephanTLavavej self-assigned this Aug 20, 2024
@StephanTLavavej StephanTLavavej removed their assignment Aug 21, 2024
@StephanTLavavej StephanTLavavej self-assigned this Aug 25, 2024
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 7782684 into microsoft:main Aug 25, 2024
@StephanTLavavej
Copy link
Member

Yay various cleanups! 🧹 🎉 🐈‍⬛

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

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants