This fixes thr bug reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108846.
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rGc4e98722ca79: [libc++] Fix std::copy and std::move for ranges with potentially overlapping…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
136 | I think we are fine with __constexpr_memmove, but the confusion here comes from the fact that we don't have a great way of conveying what the __count is: is it a number of elements or a number of bytes? This is also a problem we have in our other functions like __constexpr_memcmp_equal and others. One suggestion would be to introduce types like this: enum class _NumberOfElements : size_t {}; enum class _NumberOfBytes : size_t {}; And then we could take the appropriate parameter in __constexpr_memmove. The benefit is that callers would now explicitly pass the value so there's no ambiguity: __constexpr_memmove(dest, src, _NumberOfElements(n)) // works __constexpr_memmove(dest, src, _NumberOfBytes(n)) // doesn't work __constexpr_memmove(dest, src, n) // doesn't work We could apply the same thing to the other functions. If we do this, perhaps we should start with a refactoring of the existing functions? | |
139 | Can we replace __char_traits_move by this? | |
147 | This is the case where __is_ptr_in_range(dst, dstend, src) is true. This must be implemented using a normal forward for-loop: |-------------------------------------------------| ^ src ^ srcend ^ dst ^ dstend This is the case where __is_ptr_in_range(src, srcend, dst) is true. This must be implemented by looping backwards from srcend to src and writing from dstend to dst. |-------------------------------------------------| ^ src ^ srcend ^ dst ^ dstend Right now I think you have the logic the wrong way around. This also needs a test. The logic should be std::__is_pointer_in_range(__src, __src + __count, __dest) | |
libcxx/include/__type_traits/datasizeof.h | ||
23–24 | ||
32–33 | This fails for builtin types. Needs a test! | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp | ||
1 | What about copy_n, copy_backward and move_backward? | |
50–56 | Let's enclose this in {} to make it clear we're testing a separate property. (everywhere) |
I think this LGTM but I'd like to see again once the rebase issues have been resolved.
libcxx/include/__utility/is_pointer_in_range.h | ||
---|---|---|
36–37 | This is never true since you're passing _Tp*. is_function<_Tp> was correct, I think. Edit: Actually the overload won't match if passed a function, so let's remove this altogether. You can probably remove some includes too. | |
36–37 | If T is a ptr to member, this function doesn't match so I don't think it's too useful to have this static_assert at all: https://godbolt.org/z/8z8KPn9e5 | |
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/copy.pass.cpp | ||
51 | Since this test case and the one below it are not dependent on any template params, let's move them out of this function. It confused me at first. Probably applies to the other test files as well. |
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
149 | Some of our (and third-party) code started triggering an error related to this line after this commit. It turns out, std::vector<T>::erase() doesn't compile now, if T is move-only: https://gcc.godbolt.org/z/Txnc66M97 Is this intended? |
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
149 | Hmm, thanks for the heads up. vector<T>::erase(...) should only require that T is move assignable. In fact, it looks like this is being triggered in the std::move algorithm -- we probably don't have tests with move-only types in std::move, which is pretty embarrassing. @philnik it looks like we need to check additional constraints before calling __constexpr_memmove. Using std::move(__src[__count - 1]) here wouldn't be an option since the semantics wouldn't match those of memmove IMO. |
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
149 | If this is not intended, I'd ask for a revert, since the impact of this is rather large. |
libcxx/include/__string/constexpr_c_functions.h | ||
---|---|---|
149 | +1 this is breaking us (Chromium) also. |
I think we are fine with __constexpr_memmove, but the confusion here comes from the fact that we don't have a great way of conveying what the __count is: is it a number of elements or a number of bytes? This is also a problem we have in our other functions like __constexpr_memcmp_equal and others.
One suggestion would be to introduce types like this:
And then we could take the appropriate parameter in __constexpr_memmove. The benefit is that callers would now explicitly pass the value so there's no ambiguity:
We could apply the same thing to the other functions. If we do this, perhaps we should start with a refactoring of the existing functions?