Pointers #14
Pointers #14
Conversation
Add documentation to pointer classes
|
|
||
| class Registry | ||
| { | ||
| constexpr static std::size_t CHUNK{64}; |
There was a problem hiding this comment.
Remove unused constant.
| constexpr static std::size_t CHUNK{64}; |
|
|
||
| class Registry | ||
| { | ||
| constexpr static std::size_t CHUNK{64}; |
There was a problem hiding this comment.
What a horrendous injustice!
| * point. | ||
| */ | ||
|
|
||
| Record& take(void* target) |
There was a problem hiding this comment.
Should it really be called take? I know it's "take a record" but my first intuition was "take this out of the registry" when we actually put something in.
| * @param record The record to return. | ||
| */ | ||
|
|
||
| constexpr void put(Record& record) |
There was a problem hiding this comment.
Are we not actually deleting something from the registry rather than put something into it?
Is it more like the registry is a shelf of empty slots and if we register something, we take a record out and when we delete the record, we put it back in the registry?
There was a problem hiding this comment.
The current API uses the take()/put() nomenclature as you describe which I feel is consistent. Feel free to suggest a more intuitive alternative.
| Record& take(void* target) | ||
| { | ||
| if (!mFreeRecord) { | ||
| mRecords.resize(1 + mRecords.size()); |
There was a problem hiding this comment.
Should we not increase the storage by more than just one? Maybe double it like vector does or increase it by CHUNK? Or is this because we are still in the prototype phase and I should worry about other stuff?
| return *this; | ||
| } | ||
|
|
||
| Target& operator =(Target&& target) noexcept(std::is_nothrow_move_assignable_v<Value>) |
There was a problem hiding this comment.
Should we not make the record that is pointing to the moving-from target point to the moving-to target?
There was a problem hiding this comment.
That would make values wrapped inside Target<> behave like references:
auto a{Target<int>::create(0)};
auto b{Target<int>::create(1)};
b = a; // a and b are now 0
*b = 1; // what is a now?What you suggest would make a == 1 at the end of this code. However, Target<> is intended to have value semantics.
Pointers provide the behaviour you describe:
auto a{Target<int>::create(0)};
auto b{Target<int>::create(1)};
auto aPtr{a.ptr()};
auto bPtr{b.ptr()};
bPtr = aPtr; // *aPtr and *bPtr both refer to a and are now 0
*bPtr = 1; // *aPtr and *bPtr both refer to a and are now 1There was a problem hiding this comment.
I am suggesting it for move construction/assignment, not for copy construction/assignment...
Or would it be inconsistent and confusing then?
There was a problem hiding this comment.
auto a{Target<int>::create(0)};
auto b{Target<int>::create(1)};
b = a; // a and b are 0
*a = 1; // a is 1, b is 0
b = std::move(a); // a is indeterminate, b is 1
*a = 0; // a and b are 0Yes, I think that's worse…
There was a problem hiding this comment.
But there is a rule that by no means is a programmer allowed to touch a variable after it has been moved (see here https://en.wikipedia.org/wiki/Touch-move_rule)...
And we can even enforce that by setting the a-pointing record to nullpointer when we move from it.
There was a problem hiding this comment.
But that leaves no way to access A, the destination of the assignment. And the effect of the assignment is only observable through A-Ptrs if you don't move the value to A.
While I don't think this can be made to work usefully for move-assignment, it is completely possible for move construction. In that case, there are no destination pointers or value to worry about, and the slot of the move-source can be taken over by the new target object.
There was a problem hiding this comment.
Aww! But that's already enough to make the vector thing work, which is everything I ever wanted! I'm glad we came to an agreement in the end.
By the way, at no point did I ever consider move assignment and move construction to be kind of the same thing, why would you even think that!
There was a problem hiding this comment.
It does work with std::vector, but is still sensitive to std::is_nothrow_move_constructible as described above, and can fail silently:
auto const t0{Target<int>::create(123)};
auto const t1{std::move(t0)};performs copy-construction rather than move construction because t0 is const.
There was a problem hiding this comment.
But should we still do it then? Although I like the feature, I don't know if the benefit in this one particular case is worth the fail-silently thing and also the inconsistency with move assignment...
There was a problem hiding this comment.
I think that decision is up to the language designer
Allow re-targetting slots during move-construction.
Just for commenting on code