Avoid _Iter_diff_t in parameters of std function templates#4249
Merged
StephanTLavavej merged 4 commits intomicrosoft:mainfrom Dec 14, 2023
Merged
Avoid _Iter_diff_t in parameters of std function templates#4249StephanTLavavej merged 4 commits intomicrosoft:mainfrom
_Iter_diff_t in parameters of std function templates#4249StephanTLavavej merged 4 commits intomicrosoft:mainfrom
Conversation
`_Iter_diff_t<T>` is `typename iterator_traits<T>::difference_type` in C++17-and-earlier, and `iter_difference_t<T>` in C++20-and-later. We use it for the parameter types of a few function templates that are specified to take the former. This is (1) nicely shorter to spell than the long `iterator_traits` type, and (2) possibly a bit cheaper to compile if we can avoid instantiating `iterator_traits`. I convinced myself the difference probably wasn't observable and we made this change hoping to at least see if it would break anything. Four years later, we now know what will break: subsumption. If I pull e.g. `std::next` into a namespace with a `using` declaration and overload it with constrained function template taking the standard-specified parameters I'll get overload ambiguity on our implementation. The parameter mapping (the transformation from the template parameters to the function parameter types) differs in the two overloads so the constrained overload isn't clearly more specialized. This PR changes all function template parameters `_Iter_diff_t<meow>` to `typename iterator_traits<meow>::difference_type`. I still believe that the difference isn't observable in return types, so I've left them alone. There are also some non-local clang-format changes in `<xutility>` that I won't try to explain.
Contributor
Author
|
Me: "There's some weird formatting changes, dunno why clang-format wants them." |
Member
|
This seems tempting to regress during maintenance without test coverage - it's the sort of "why are we saying verbose thing when we could use fewer words" that seems good to cleanup. So I'd like to see test coverage added for this. |
StephanTLavavej
approved these changes
Dec 14, 2023
Member
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Member
|
Thanks for making a difference! 😹 🚀 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_Iter_diff_t<T>istypename iterator_traits<T>::difference_typein C++17-and-earlier, anditer_difference_t<T>in C++20-and-later. We use it for the parameter types of a few function templates that are specified to take the former. This is (1) nicely shorter to spell than the longiterator_traitstype, and (2) possibly a bit cheaper to compile if we can avoid instantiatingiterator_traits. I convinced myself the difference probably wasn't observable and we made this change hoping to at least see if it would break anything.Four years later, we now know what will break: subsumption. If I pull e.g.
std::nextinto a namespace with ausingdeclaration and overload it with constrained function template taking the standard-specified parameters I'll get overload ambiguity on our implementation. The parameter mapping (the transformation from the template parameters to the function parameter types) differs in the two overloads so the constrained overload isn't clearly more specialized.This PR changes all function template parameters
_Iter_diff_t<meow>totypename iterator_traits<meow>::difference_type. I still believe that the difference isn't observable in return types, so I've left them alone. I don't think we're likely to regress this, so I've added no test coverage. Shout - or you know, leave a review comment - if you disagree.Fixes DevCom-10532126 / VSO-1925201 / AB#1925201.