Re: [C++ PATCH] OVERLOAD iterators #1

[Martin Sebor]

On 05/16/2017 01:20 PM, Tim Song wrote:

On Tue, May 16, 2017 at 2:50 PM, Martin Sebor  wrote:

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.



Huh? The implicitly declared copy assignment operator in a derived
class will always hide the assignment operators from the base class.


Ah, yes, you're right about the copy assignment.  Thanks for
setting me straight!  Clearly my C++ has become so rusty from
working on a C++ compiler that I forgot this essential bit
(making Nathan's C++-ification of GCC source code that much
more important! ;)  The general point is still valid that
the other base members that return a reference to the base
type will have the slicing effect above.

Martin

Re: [C++ PATCH] OVERLOAD iterators #1

[Martin Sebor]

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 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 
  bool opearator== (const forward_iterator ,
const forward_iterator )
  {
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;

  struct ovl_iterator_tag;
  typedef forward_iterator 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

Re: [C++ PATCH] OVERLOAD iterators #1

[Nathan Sidwell]

On 05/16/2017 03:27 PM, Tim Song wrote:


https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool


Oh, I see.  I'm not going to stop someone adding it, but for me, it's 
quite far down my priority list.



I suspect that we don't want adding two ovl_iterators together to compile.


Why would anyone write that :)

nathan
--
Nathan Sidwell

Re: [C++ PATCH] OVERLOAD iterators #1

[Tim Song]

On Tue, May 16, 2017 at 3:24 PM, Nathan Sidwell  wrote:
> On 05/16/2017 03:20 PM, Tim Song wrote:
>
>> Also, operator bool() seems suspect. Consider the safe bool idiom?
>
> ?
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool

I suspect that we don't want adding two ovl_iterators together to compile.

Re: [C++ PATCH] OVERLOAD iterators #1

[Nathan Sidwell]

On 05/16/2017 03:20 PM, Tim Song wrote:


Also, operator bool() seems suspect. Consider the safe bool idiom?

?


And is it intended that tree implicitly converts to both iterators, or
should those constructors be explicit?


Maybe.  Not caused a problem in practice -- never passing iterators 
around, so they're always effectively explicit.


nathan

--
Nathan Sidwell

Re: [C++ PATCH] OVERLOAD iterators #1

[Tim Song]

On Tue, May 16, 2017 at 2:50 PM, Martin Sebor  wrote:
> 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.
>

Huh? The implicitly declared copy assignment operator in a derived
class will always hide the assignment operators from the base class.

Also, operator bool() seems suspect. Consider the safe bool idiom?

And is it intended that tree implicitly converts to both iterators, or
should those constructors be explicit?

Re: [C++ PATCH] OVERLOAD iterators #1

[Nathan Sidwell]

On 05/16/2017 03:05 PM, Nathan Sidwell wrote:


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?


To be clear, I mean your suggestion works for me, not that the current 
names are intuitive to me (but that's also true as I've been exposed to 
them for several months)


nathan


--
Nathan Sidwell

Re: [C++ PATCH] OVERLOAD iterators #1

[Nathan Sidwell]

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.



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?


nathan

--
Nathan Sidwell

Re: [C++ PATCH] OVERLOAD iterators #1

[Martin Sebor]

On 05/16/2017 08:50 AM, Nathan Sidwell wrote:

This patch implements new iterators for OVERLOADs.  There are two
iterators:
ovl_iterator for a plain iterator, that held on a binding
lkp_iterator for the overload set returned by lookup.

To use them simply:
  for (lkp_iterator iter (INIT); iter; ++iter)
{ tree fn = *iter;
  ... }

Currently the latter simply defers to the former, but I'll be changing
lookup so that it can return an overload of overloads.  (Right now it
simply flattens things, which is sub-optimal).

This changes the easier cases of iteration to use these.  I'll be
working through the other cases to convert them.


I've often wished for nice interfaces like this to get away from
the low level macros and make working with trees feel at least
a little bit like using C++ :)  Thanks for making it possible!

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?

E.g., define both pre-increment and post-increment, define
the equality operator (as a non-member), etc., based on
C++ Forward Iterator requirements.

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.

(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.)

Martin

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?