Added support for range based loop - #2
Conversation
joancarreras
commented
Oct 13, 2021
joancarreras
commented
- TBD
There was a problem hiding this comment.
I understand this iterator inheritance is the whole point of the implementation. Since ArrayIterator_ class does not have members there should not be any memory leak because of parent's non-virtual destructor, so I'm ok with it.
However, I would suggest to address the two other comments
|
|
||
| // Array iterator for range-loop | ||
| template <typename BaseIterator_, typename Ref_> | ||
| class ArrayIterator_ : public BaseIterator_ |
There was a problem hiding this comment.
it is normally considered bad practice to inherit from STL (in this case vector's iterator) as they do not declare dtor virtual on purpose
There was a problem hiding this comment.
The implementation of the whole class has changed in order to be unrelated to the vector<unique_ptr> (because it reveals the current way to store values contained in the JSONValue implementation, something inherent in the implementation but not in the interface)
|
|
||
| Ref_ operator*() const | ||
| { | ||
| const BaseIterator_ it = *this; |
There was a problem hiding this comment.
original implementation does this function in one line
reference operator*() const
{ // return designated object
return ((reference)**(_Mybase *)this);
}
I recommend condensing it also in one line to reduce code
| { | ||
| public: | ||
|
|
||
| virtual ~ArrayIterator_() = default; |
There was a problem hiding this comment.
not needed as this is a final class, adding this virtual method implies allocating vtable unnecessarily
There was a problem hiding this comment.
Yes, I agree, thanks! (as a previous approach I had considered making this class as an 'IArrayIterator' interface; because of this the 'virtual' in the destructor has remained here without being necessary now)
Redesign of ArrayIterator class for range base loops, in order to lean on the existing virtual method IJSONValue::getArrayValue(), avoiding to mention vector<unique_ptr<IJSONValue>>.
2 participants
