KEMBAR78
Use `_Verify_ranges_do_not_overlap` in more places by AlexGuteniev · Pull Request #5495 · microsoft/STL · GitHub
Skip to content

Conversation

@AlexGuteniev
Copy link
Contributor

Resolves #5472.

Only used the function in the places where it is a perfect match. So no ranges::swap_ranges with two sentinels, and no std::copy etc with weaker precondition.

I was very surprised to learn that copy_if / remove_copy / remove_copy_if / unique_copy require non-overlap as follows:

The ranges [first, last) and [result, result + (last - first)) do not overlap.

This implies that the destination range may fit the same element number as the source range contains. I thought only fitting the actually returned number of elements is enough.

Overall, now as I've done this, I'm not even sure if it is at all useful.

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner May 11, 2025 18:51
@github-project-automation github-project-automation bot moved this to Initial Review in STL Code Reviews May 11, 2025
and you can see where I copied the death test from
@AlexGuteniev
Copy link
Contributor Author

libc++ tests fail specifically due to this:

This implies that the destination range may fit the same element number as the source range contains. I thought only fitting the actually returned number of elements is enough.

They have:

      std::array<S, 4> in = {{{4, 2}, {1, 3}, {3, 4}, {3, 5}}};
      std::array<S, 2> out;
      auto ret = std::ranges::copy_if(in.begin(), in.end(), out.begin(), [](int i) { return i == 3; }, &S::val);

https://github.com/llvm/llvm-project/blob/79210feb2993ff9a79ef11f8a7016a527d4fcf22/libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy_if.pass.cpp#L160-L162

@AlexGuteniev
Copy link
Contributor Author

LLVM-139464 created.

Now that copycats are not enforced to have non-overlap, this looks even less useful.

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label May 11, 2025
@StephanTLavavej StephanTLavavej self-assigned this May 11, 2025
@AlexGuteniev
Copy link
Contributor Author

I was very surprised to learn that copy_if / remove_copy / remove_copy_if / unique_copy require non-overlap as follows:

I think that standard shoould have a weaker precondition here. Just dest not in [first, last)

@github-project-automation github-project-automation bot moved this from Initial Review to Work In Progress in STL Code Reviews May 14, 2025
@StephanTLavavej StephanTLavavej removed their assignment May 14, 2025
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in STL Code Reviews May 15, 2025
@StephanTLavavej StephanTLavavej self-assigned this May 15, 2025
@StephanTLavavej StephanTLavavej removed their assignment May 16, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in STL Code Reviews May 16, 2025
@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews May 16, 2025
@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 c8ae40b into microsoft:main May 17, 2025
39 checks passed
@github-project-automation github-project-automation bot moved this from Merging to Done in STL Code Reviews May 17, 2025
@StephanTLavavej
Copy link
Member

⛰️ ❌ 🏔️

@AlexGuteniev AlexGuteniev deleted the do-not-overlap branch May 17, 2025 05:02
@AlexGuteniev
Copy link
Contributor Author

I was very surprised to learn that copy_if / remove_copy / remove_copy_if / unique_copy require non-overlap as follows:

I think that standard shoould have a weaker precondition here. Just dest not in [first, last)

For the record, this is now LWG-4262

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.

<algorithm>: use _Verify_ranges_do_not_overlap function in more places

2 participants