Bug 24481 – retro no longer works with types that support assignment but not moving

Status
RESOLVED
Resolution
FIXED
Severity
regression
Priority
P1
Component
phobos
Product
D
Version
D2
Platform
All
OS
All
Creation time
2024-04-04T00:46:32Z
Last change time
2024-05-03T18:52:28Z
Keywords
pull
Assigned to
No Owner
Creator
Jonathan M Davis

Comments

Comment #0 by issues.dlang — 2024-04-04T00:46:32Z
https://github.com/dlang/phobos/pull/8721#issuecomment-2035620659 This PR results in issues at Weka with retro for a struct that has void opAssign()(auto ref const(typeof(this)) that) const defined, i.e. a struct that allows const assignment. The problem is that assignment is allowed, but move is not (because it would be a memcpy to const memory). The cause of the problem: retro checks for hasAssignableElements, but then does a move rather than an assignment. https://github.com/dlang/phobos/pull/8721/files#diff-705ed5d9f4ea4f9dadcb07d7116df8dc8397e851639a28538dc0b1d6fa446f56L312-R318 Testcase (https://d.godbolt.org/z/z6KMY6nYf): import std.range; struct Handle { int[5] entries; void opAssign()(auto ref const(typeof(this)) that) const { } } const(Handle)[100] arr; auto foo() { return arr[0..5].retro; } the problem is fixed by reverting the lines for retro.
Comment #1 by dlang-bot — 2024-04-04T05:21:15Z
@jmdavis created dlang/phobos pull request #8969 "Fix bugzilla issue 24481: retro stopped working" fixing this issue: - Fix bugzilla issue 24481: retro stopped working In an attempt make it so that non-copyable types worked with some of the functions in std/range/package.d, they were made to use moves instead of assignment, which broke the code for types which work with assignment but not moves (which affected the folks at Weka). The code checked for assignment but not whether move could be used, and that didn't change when the code was changed to use move, meaning that the checks didn't match what the code was actually doing. So, to support both the non-copyable types and the ones that can be assigned to but not moved to, this changes the code so that it uses a static if to check whether using move compiles, branching the code based on whether move works on not. If move works, it's used. If it doesn't, then assignment is used like used to be the case. So, in theory, the code that worked previously works again, and the newer functionality of being able to use non-copyable types with this code continues to work. Discussion here: https://github.com/dlang/phobos/pull/8721 https://github.com/dlang/phobos/pull/8969
Comment #2 by dlang-bot — 2024-04-29T09:33:37Z
dlang/phobos pull request #8969 "Fix bugzilla issue 24481: retro stopped working" was merged into stable: - f66f2b22a69005f7a1ba9ea40a7ad814d840ef36 by Jonathan M Davis: Fix bugzilla issue 24481: retro stopped working In an attempt make it so that non-copyable types worked with some of the functions in std/range/package.d, they were made to use moves instead of assignment, which broke the code for types which work with assignment but not moves (which affected the folks at Weka). The code checked for assignment but not whether move could be used, and that didn't change when the code was changed to use move, meaning that the checks didn't match what the code was actually doing. So, to support both the non-copyable types and the ones that can be assigned to but not moved to, this changes the code to use core.lifetime.forward which will move the argument if it can and assign otherwise. So ,the code that worked previously should work again, and the newer functionality of being able to use non-copyable types with this code should continue to work. Discussion here: https://github.com/dlang/phobos/pull/8721 https://github.com/dlang/phobos/pull/8969
Comment #3 by dlang-bot — 2024-05-03T18:52:28Z
dlang/phobos pull request #8998 "merge stable" was merged into master: - ffe00ebdc38ee803aab25e8032ca5828e06fafcf by Jonathan M Davis: Fix bugzilla issue 24481: retro stopped working In an attempt make it so that non-copyable types worked with some of the functions in std/range/package.d, they were made to use moves instead of assignment, which broke the code for types which work with assignment but not moves (which affected the folks at Weka). The code checked for assignment but not whether move could be used, and that didn't change when the code was changed to use move, meaning that the checks didn't match what the code was actually doing. So, to support both the non-copyable types and the ones that can be assigned to but not moved to, this changes the code to use core.lifetime.forward which will move the argument if it can and assign otherwise. So ,the code that worked previously should work again, and the newer functionality of being able to use non-copyable types with this code should continue to work. Discussion here: https://github.com/dlang/phobos/pull/8721 https://github.com/dlang/phobos/pull/8998