Make v_array internals private, don't allow begin/end to be modified #2874
Conversation
There was a problem hiding this comment.
A few questions and an out of place review of the previous PR that introduced actual_resize.
That name is pretty bad as it's not obvious what it does. I think a better naming for those placeholders are on the form xxx_but_yyy or xxx_with_yyy so, for example, it could be resize_with_stl_behavior, which still keeps the sense of impropriety of its existence.
Finally, that PR makes VW 10 promises but didn't record it in the VW 10 project board.
| @@ -30,16 +30,16 @@ void features::clear() | |||
| void features::truncate_to(const features_value_iterator& pos) | |||
| { | |||
| auto i = pos._begin - values.begin(); | |||
There was a problem hiding this comment.
Could we make this safe? If pos didn't come from values we have UD.
There was a problem hiding this comment.
features_value_iterator is created with a ptr, so we'd likely need to enforce in its creation. In this scope I do not think we have to information to make this safe. I think this should be handled separately
| rfs.space_names.end() = rfs.space_names.begin() + lrq.orig_size[right]; | ||
| } | ||
| rfs.values.actual_resize(lrq.orig_size[right]); | ||
| if (all.audit || all.hash_inv) { rfs.space_names.resize(lrq.orig_size[right]); } |
There was a problem hiding this comment.
rfs.space_names is a std::vector Because the destructor is handled by std::vectors resize. I don't know how this code worked before if at all...
| @@ -75,12 +75,9 @@ struct v_array<T, typename std::enable_if<std::is_trivially_copyable<T>::value>: | |||
| memmove(&_begin[idx + width], &_begin[idx], (size() - (idx + width)) * sizeof(T)); | |||
| } | |||
|
|
|||
| public: | |||
| // private: | |||
There was a problem hiding this comment.
Welp it is private for real now
Required #2873