<deque>: Fix iterator arithmetic
#4049
Conversation
|
Hmm, I think it will be horrible to have two |
| // NB: _Mapsize and _Block_size are guaranteed to be powers of 2 | ||
| return (_Off / _Block_size) & (_Mapsize - 1); | ||
| return static_cast<_Map_difference_type>((_Off / _Block_size) & (_Mapsize - 1)); |
There was a problem hiding this comment.
I think we should put off every cast-to-_Map_difference_type to only when having to add to a pointer.
stl/inc/deque
Outdated
|
|
||
| reference _At(size_type _Idx) noexcept { | ||
| const auto _Block = _Getblock(_Idx); | ||
| const auto _Off = static_cast<difference_type>(_Idx % _Block_size); |
There was a problem hiding this comment.
If I'm correct, this difference_type comes from _Alty_traits::difference_type, which has a different source from _Map_difference_type (_Al[p]ty_traits).
My suggestions are:
- not changing return type of
_Getblock. - this function should be
const size_type _Block = _Getblock(_Idx);
const auto _Off = static_cast<_Map_difference_type>(_Idx(or "_Off") % _Block_size);
return _Map[_Block][_Off];
There was a problem hiding this comment.
If I'm correct, this
difference_typecomes from_Alty_traits::difference_type, which has a different source from_Map_difference_type(_Al[p]ty_traits).
This is right.
My suggestions are:
- not changing return type of
_Getblock.- this function should be
const size_type _Block = _Getblock(_Idx); const auto _Off = static_cast<_Map_difference_type>(_Idx(or "_Off") % _Block_size); return _Map[_Block][_Off];
These suggestions don't seem correct. The new test case will be rejected with these applied.
There was a problem hiding this comment.
The new test case will be rejected with these applied.
Yes, I mistook the types here :(
| @@ -74,7 +72,7 @@ public: | |||
| } | |||
|
|
|||
| _Deque_unchecked_const_iterator& operator+=(const difference_type _Off) noexcept { | |||
There was a problem hiding this comment.
As a side note, these different_type comes from _Alty_traits.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Thanks! I think this needs only one maintainer approval. |
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
Thanks for improving our fancy pointer support! 🧐 🎩 🎉 |
Currently we sometimes directly add a
size_typevalue to a fancy pointer, which is not always supported. Also, the difference type ofpointerand_Mapptrmay be different (which is contrived though). I think we shouldstatic_castto the correct difference type in iterator arithmetic.The return value of
_Getblockis always used in the subscipt, so I guess it's OK to change the return type to the difference type.Driven-by: avoid seemingly redundant indirection in
operator[]andat. (Perhaps we should also do this forfrontandback).