On 05/16/2017 01:05 PM, Nathan Sidwell wrote:
Martin,
Thanks for taking a look. There's a whole patch series I hope to land
over the next couple of weeks. Things may be clearer at that point.
Just a couple of suggestions, It looks like the classes model
the concept of Forward Iterator. May I suggest to make them
model it more closely and make them behave in unsurprising
ways to those familiar with the concept?
Sure -- I'm an STL weenie though. I only defined the members that I
needed for what I wanted to do. (Originally I had hoped to turn
OVERLOADS into vectors, but that didn't work out for reasons that I'll
get to in later patches.)
I'm not sure I understand why the ovl_iterator assignment needs
to be provided but if it does, not also defining one on the derived
class will call the base and return a reference to the base, making
the result no longer suitable where the derived is needed. This
is true for any other base members that return [a reference to]
the base type.
The assignment is needed when the 2-d iterator stuff lands. It becomes
more complex then. But I think your point may still be valid.
(If distinct types for iterators modeling the same concept
are necessary (or helpful) I would actually suggest to avoid
inheritance and instead introduce a generic iterator as
a template, and as many distinct [implicit] specializations
as necessary, with typedefs for each to make them look and
feel like classes.)
Right, that was my initial idea, but the polymorphism of tree didn't
really help. The compiler can't deduce at compile time from a tree
node, even if you know it's an OVERLOAD, as to which iterator you want.
I'm not sure I explained it clearly enough (or maybe I don't
understand the problem you are referring to). Let me try to
sketch out what I meant (completely off the cuff):
template <class Tag>
class forward_iterator {
tree ovl;
public:
// this perhaps should be declared explicit
forward_iterator (tree);
forward_iterator& operator++ ();
forward_iterator operator++ (int) {
forward_iterator tmp (*this);
++*this;
return tmp;
}
// ...
};
template <class T>
bool opearator== (const forward_iterator<T> &lhs,
const forward_iterator<T> &rhs)
{
return lhs.ovl == rhs.ovl; // or whatever is appropriate
}
// repeat the above for other equality and relational operators
struct lkp_iterator_tag;
typedef forward_iterator<lkp_iterator_tag> lkp_iterator;
struct ovl_iterator_tag;
typedef forward_iterator<ovl_iterator_tag> ovl_iterator;
I.e., only one implementation template is needed and it gets
instantiated on a unique tag to make the iterators distinct
from one another. This avoids the slicing problem with
the derivation.
PS More descriptive names would be a nice as well (neither
lkp_ nor ovl_ is intuitive enough at first glance.) Maybe
lookup_iter and overload_iter?
Works for me -- I just noticed we had things like vec_iterator already,
and continued that naming. It was only recently I renamed lkp_iterator
from ovl2_iterator! Let's get this landed before renaming things though?
I'm sure it's easy to get used to. It's just a tiny bit easier
to read for those who see it for the first time. Whatever works
Martin
