Feature: Implement Rebindable for non-class/non-array types.#8722
Conversation
|
Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#8722" |
|
Please add either a changelog or an issue this closes. |
FeepingCreature
force-pushed
the
feature/rebindable-for-structs
branch
from
4624a54 to
3c2ecf3
Compare
|
Added a changelog entry. |
FeepingCreature
force-pushed
the
feature/rebindable-for-structs
branch
2 times, most recently
from
8e543c4 to
41dc73c
Compare
|
Ping @RazvanN7 @atilaneves since you reviewed the predecessor, @wilzbach @ntrel as fyi (predecessor PRs) |
| @safe unittest | ||
| /// ditto | ||
| struct Rebindable(T) | ||
| if (!is(T == class) && !is(T == interface) && !isDynamicArray!T && !isAssociativeArray!T) |
There was a problem hiding this comment.
Are we really targeting all types? Or is this another way to say T == struct || T == basic type ? T could also be an AliasSeq, is that supposed to be supported?
There was a problem hiding this comment.
T cannot be an AliasSeq because AliasSeq (except with one member) doesn't match to T. Everything else is targeted, sure. What wouldn't be?
It should work with anything that can be copied and returned from a function.
There was a problem hiding this comment.
Isn't the condition cleaner and more expressive if it states the targeted types rather than negating the ones that are not?
There was a problem hiding this comment.
Right, but I genuinely don't know which types it wouldn't work for (void, I guess?), so the condition would just be a list of "every type in D except those handled by the other Rebindable implementation." Which is better expressed by inverting the condition from that type.
Enumerating types makes sense if something in the implementation is specific to those types. It isn't - sizeof, alignof and pointer casts work with everything.
FeepingCreature
force-pushed
the
feature/rebindable-for-structs
branch
from
41dc73c to
e3d7e76
Compare
|
ping |
dkorpel
left a comment
dkorpel
left a comment
There was a problem hiding this comment.
@atilaneves Do you approve this feature?
| /// | ||
| @system unittest | ||
| { | ||
| class CustomOpEq |
There was a problem hiding this comment.
Why are the tests for a rebindable class removed?
FeepingCreature
force-pushed
the
feature/rebindable-for-structs
branch
from
6fbea55 to
46a04b0
Compare
| /** | ||
| * Constructs a `Rebindable` from a given value. | ||
| */ | ||
| this(T value) @trusted |
There was a problem hiding this comment.
What if the type can't be copied?
|
Nice. Given that there are two implementations, how do we ensure that they both act the same way? Is there any difference in the API? |
|
Yes, there are critical API differences: non-class Rebindable cannot offer |
|
I think it's ok to not offer some functionality depending on type. |
|
Yeah my hope is that non-copyable types and immutable types will basically be non-overlapping categories. |
|
ping |
FeepingCreature
force-pushed
the
feature/rebindable-for-structs
branch
from
46a04b0 to
c7ca5d0
Compare
|
Rebased in hopes the tests pass now. edit: Cannot reproduce the buildkite failure locally. |
Note that you cannot access a reference to the contained value in struct Rebindable.
FeepingCreature
force-pushed
the
feature/rebindable-for-structs
branch
from
c7ca5d0 to
d6f0f70
Compare
|
Pushed bogus change to hopefully rerun the buildkite. |
|
Okay, ddox failed again. Two times is enemy action. What's going on there? It doesn't happen if I run locally, and there's no error in the build output, only "exit code 1". Any ideas? |
|
Looks unrelated, other PRs have it as well. Perhaps @RazvanN7 can force merge this |
|
OMG it actually happened!! starts prepping follow-up PRs |
6 participants
Implement
Rebindable!TwhereTis any type, including const and immutable.Arrays, objects and interfaces are still handled by the old
Rebindableimplementation.Note that you cannot access a reference to the contained value in
Rebindable. This is unavoidable; otherwise you could violate constness. For this and related information, see my 2022 DConf talk https://www.youtube.com/watch?v=eGX_fxlig8IAs opposed to the previous PR, this implementation deliberately contains the least amount of code to do something useful. As per review feedback, this PR is a simplified version of #8674 , which was a rewrite of #6136, which was itself a revival of #4363. This time for sure!