[C++ PATCH] OVERLOAD iterators #1

[C++ PATCH] OVERLOAD iterators #1

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

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.

nathan
-- 
Nathan Sidwell

[-- Attachment #2: ovl-iter-1.diff --]
[-- Type: text/x-patch, Size: 14216 bytes --]

2017-05-16  Nathan Sidwell  <nathan@acm.org>

	* cp-tree.h (class ovl_iterator, class lkp_iterator): New OVERLOAD
	iterators.
	(MAYBE_BASELINK_FUNCTIONS): New.
	* constraint.cc	(resolve_constraint_check): Use lkp_iterator.
	* decl2.c (maybe_warn_sized_delete): Use ovl_iterator.
	* lambda.c (maybe_generic_this_capture): Use lkp_iterator.
	* method.c (inherited_ctor_binfo): Use ovl_iterator.
	(binfo_inherited_from): Likewise.
	* parser.c (lookup_literal_operator): Use lkp_iterator.
	* pt.c (iterative_hash_template_arg): Use lkp_iterator.
	(print_candidates_1): Likewise.
	(determine_specialization): Likewise.
	(resolve_overloaded_unification): Likewise.
	(resolve_nondeduced_context): Likewise.
	(type_dependent_expression_p): Likewise.
	(dependent_template_p): Likewise.
	* ptree.c (cxx_print_xnode): Likewise.
	* semantics.c (omp_reduction_lookup): Use lkp_iterator.
	(classtype_has_nothrow_assign_or_copy_p): Use ovl_iterator.
	* typeck.c (check_template_keyword): Use lkp_iterator.

Index: constraint.cc
===================================================================
--- constraint.cc	(revision 248109)
+++ constraint.cc	(working copy)
@@ -204,10 +204,10 @@ resolve_constraint_check (tree ovl, tree
 {
   int nerrs = 0;
   tree cands = NULL_TREE;
-  for (tree p = ovl; p != NULL_TREE; p = OVL_NEXT (p))
+  for (lkp_iterator iter (ovl); iter; ++iter)
     {
       // Get the next template overload.
-      tree tmpl = OVL_CURRENT (p);
+      tree tmpl = *iter;
       if (TREE_CODE (tmpl) != TEMPLATE_DECL)
         continue;
 
Index: cp-tree.h
===================================================================
--- cp-tree.h	(revision 248109)
+++ cp-tree.h	(working copy)
@@ -636,6 +636,62 @@ struct GTY(()) tree_overload {
   tree function;
 };
 
+/* Iterator for a 1 dimensional overload. */
+
+class ovl_iterator 
+{
+  tree ovl;
+
+ public:
+  ovl_iterator (tree o)
+  :ovl (o)
+    {}
+
+  ovl_iterator &operator= (const ovl_iterator &from)
+  {
+    ovl = from.ovl;
+
+    return *this;
+  }
+
+ public:
+  operator bool () const
+  {
+    return ovl;
+  }
+  ovl_iterator &operator++ ()
+  {
+    ovl = TREE_CODE (ovl) != OVERLOAD ? NULL_TREE : OVL_CHAIN (ovl);
+    return *this;
+  }
+  tree operator* () const
+  {
+    tree fn = TREE_CODE (ovl) != OVERLOAD ? ovl : OVL_FUNCTION (ovl);
+
+    /* Check this is not an unexpected 2-dimensional overload.  */
+    gcc_checking_assert (TREE_CODE (fn) != OVERLOAD);
+
+    return fn;
+  }
+};
+
+/* Iterator over a (potentially) 2 dimensional overload, which is
+   produced by name lookup.  */
+
+/* Note this is currently a placeholder, as the name-lookup changes
+   are not yet committed.  */
+
+class lkp_iterator : public ovl_iterator
+{
+  typedef ovl_iterator parent;
+
+ public:
+  lkp_iterator (tree o)
+    : parent (o)
+  {
+  }
+};
+
 struct GTY(()) tree_template_decl {
   struct tree_decl_common common;
   tree arguments;
@@ -653,6 +709,10 @@ struct GTY(()) tree_template_decl {
    a TEMPLATE_DECL, an OVERLOAD, or a TEMPLATE_ID_EXPR.  */
 #define BASELINK_FUNCTIONS(NODE) \
   (((struct tree_baselink*) BASELINK_CHECK (NODE))->functions)
+/* If T is a BASELINK, grab the functions, otherwise just T, which is
+   expected to already be a (list of) functions.  */
+#define MAYBE_BASELINK_FUNCTIONS(T) \
+  (BASELINK_P (T) ? BASELINK_FUNCTIONS (T) : T)
 /* The BINFO in which the search for the functions indicated by this baselink
    began.  This base is used to determine the accessibility of functions
    selected by overload resolution.  */
Index: decl2.c
===================================================================
--- decl2.c	(revision 248109)
+++ decl2.c	(working copy)
@@ -4390,10 +4390,10 @@ maybe_warn_sized_delete (enum tree_code
   tree sized = NULL_TREE;
   tree unsized = NULL_TREE;
 
-  for (tree ovl = IDENTIFIER_GLOBAL_VALUE (cp_operator_id (code));
-       ovl; ovl = OVL_NEXT (ovl))
+  for (ovl_iterator iter (IDENTIFIER_GLOBAL_VALUE (cp_operator_id (code)));
+       iter; ++iter)
     {
-      tree fn = OVL_CURRENT (ovl);
+      tree fn = *iter;
       /* We're only interested in usual deallocation functions.  */
       if (!usual_deallocation_fn_p (fn))
 	continue;
Index: lambda.c
===================================================================
--- lambda.c	(revision 248109)
+++ lambda.c	(working copy)
@@ -841,18 +841,15 @@ maybe_generic_this_capture (tree object,
 	bool id_expr = TREE_CODE (fns) == TEMPLATE_ID_EXPR;
 	if (id_expr)
 	  fns = TREE_OPERAND (fns, 0);
-	for (; fns; fns = OVL_NEXT (fns))
-	  {
-	    tree fn = OVL_CURRENT (fns);
 
-	    if ((!id_expr || TREE_CODE (fn) == TEMPLATE_DECL)
-		&& DECL_NONSTATIC_MEMBER_FUNCTION_P (fn))
-	      {
-		/* Found a non-static member.  Capture this.  */
-		lambda_expr_this_capture (lam, true);
-		break;
-	      }
-	  }
+	for (lkp_iterator iter (fns); iter; ++iter)
+	  if ((!id_expr || TREE_CODE (*iter) == TEMPLATE_DECL)
+	      && DECL_NONSTATIC_MEMBER_FUNCTION_P (*iter))
+	    {
+	      /* Found a non-static member.  Capture this.  */
+	      lambda_expr_this_capture (lam, true);
+	      break;
+	    }
       }
 }
 
Index: method.c
===================================================================
--- method.c	(revision 248109)
+++ method.c	(working copy)
@@ -538,9 +538,9 @@ inherited_ctor_binfo (tree binfo, tree f
     return binfo;
 
   tree results = NULL_TREE;
-  for (; inh; inh = OVL_NEXT (inh))
+  for (ovl_iterator iter (inh); iter; ++iter)
     {
-      tree one = inherited_ctor_binfo_1 (binfo, OVL_CURRENT (inh));
+      tree one = inherited_ctor_binfo_1 (binfo, *iter);
       if (!results)
 	results = one;
       else if (one != results)
@@ -593,9 +593,9 @@ binfo_inherited_from (tree binfo, tree i
 {
   /* inh is an OVERLOAD if we inherited the same constructor along
      multiple paths, check all of them.  */
-  for (; inh; inh = OVL_NEXT (inh))
+  for (ovl_iterator iter (inh); iter; ++iter)
     {
-      tree fn = OVL_CURRENT (inh);
+      tree fn = *iter;
       tree base = DECL_CONTEXT (fn);
       tree base_binfo = NULL_TREE;
       for (int i = 0; BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
Index: parser.c
===================================================================
--- parser.c	(revision 248109)
+++ parser.c	(working copy)
@@ -4061,16 +4061,16 @@ cp_parser_string_literal (cp_parser *par
 static tree
 lookup_literal_operator (tree name, vec<tree, va_gc> *args)
 {
-  tree decl, fns;
+  tree decl;
   decl = lookup_name (name);
   if (!decl || !is_overloaded_fn (decl))
     return error_mark_node;
 
-  for (fns = decl; fns; fns = OVL_NEXT (fns))
+  for (lkp_iterator iter (decl); iter; ++iter)
     {
       unsigned int ix;
       bool found = true;
-      tree fn = OVL_CURRENT (fns);
+      tree fn = *iter;
       tree parmtypes = TYPE_ARG_TYPES (TREE_TYPE (fn));
       if (parmtypes != NULL_TREE)
 	{
Index: pt.c
===================================================================
--- pt.c	(revision 248109)
+++ pt.c	(working copy)
@@ -1746,8 +1746,8 @@ iterative_hash_template_arg (tree arg, h
       return val;
 
     case OVERLOAD:
-      for (; arg; arg = OVL_NEXT (arg))
-	val = iterative_hash_template_arg (OVL_CURRENT (arg), val);
+      for (lkp_iterator iter (arg); iter; ++iter)
+	val = iterative_hash_template_arg (*iter, val);
       return val;
 
     case CONSTRUCTOR:
@@ -1926,15 +1926,15 @@ print_candidates_1 (tree fns, char **str
     for (; fns; fns = TREE_CHAIN (fns))
       print_candidates_1 (TREE_VALUE (fns), str, more || TREE_CHAIN (fns));
   else
-    while (fns)
+    for (lkp_iterator iter (fns); iter;)
       {
-	tree cand = OVL_CURRENT (fns);
+	tree cand = *iter;
+	++iter;
 
-	fns = OVL_NEXT (fns);
 	const char *pfx = *str;
 	if (!pfx)
 	  {
-	    if (more || fns)
+	    if (more || iter)
 	      pfx = _("candidates are:");
 	    else
 	      pfx = _("candidate is:");
@@ -2102,9 +2102,9 @@ determine_specialization (tree template_
       if (targs != error_mark_node)
         templates = tree_cons (targs, fns, templates);
     }
-  else for (; fns; fns = OVL_NEXT (fns))
+  else for (lkp_iterator iter (fns); iter; ++iter)
     {
-      tree fn = OVL_CURRENT (fns);
+      tree fn = *iter;
 
       if (TREE_CODE (fn) == TEMPLATE_DECL)
 	{
@@ -19379,9 +19379,9 @@ resolve_overloaded_unification (tree tpa
       tree expl_subargs = TREE_OPERAND (arg, 1);
       arg = TREE_OPERAND (arg, 0);
 
-      for (; arg; arg = OVL_NEXT (arg))
+      for (lkp_iterator iter (arg); iter; ++iter)
 	{
-	  tree fn = OVL_CURRENT (arg);
+	  tree fn = *iter;
 	  tree subargs, elem;
 
 	  if (TREE_CODE (fn) != TEMPLATE_DECL)
@@ -19420,15 +19420,17 @@ resolve_overloaded_unification (tree tpa
        not just the function on its own.  */
     return false;
   else
-    for (; arg; arg = OVL_NEXT (arg))
-      if (try_one_overload (tparms, targs, tempargs, parm,
-			    TREE_TYPE (OVL_CURRENT (arg)),
-			    strict, sub_strict, addr_p, explain_p)
-	  && (!goodfn || !decls_match (goodfn, OVL_CURRENT (arg))))
-	{
-	  goodfn = OVL_CURRENT (arg);
-	  ++good;
-	}
+    for (lkp_iterator iter (arg); iter; ++iter)
+      {
+	tree fn = *iter;
+	if (try_one_overload (tparms, targs, tempargs, parm, TREE_TYPE (fn),
+			      strict, sub_strict, addr_p, explain_p)
+	    && (!goodfn || !decls_match (goodfn, fn)))
+	  {
+	    goodfn = fn;
+	    ++good;
+	  }
+      }
 
   /* [temp.deduct.type] A template-argument can be deduced from a pointer
      to function or pointer to member function argument if the set of
@@ -19510,9 +19512,9 @@ resolve_nondeduced_context (tree orig_ex
       tree badfn = NULL_TREE;
       tree badargs = NULL_TREE;
 
-      for (; arg; arg = OVL_NEXT (arg))
+      for (lkp_iterator iter (arg); iter; ++iter)
 	{
-	  tree fn = OVL_CURRENT (arg);
+	  tree fn = *iter;
 	  tree subargs, elem;
 
 	  if (TREE_CODE (fn) != TEMPLATE_DECL)
@@ -23926,12 +23928,10 @@ type_dependent_expression_p (tree expres
       gcc_assert (TREE_CODE (expression) == OVERLOAD
 		  || TREE_CODE (expression) == FUNCTION_DECL);
 
-      while (expression)
-	{
-	  if (type_dependent_expression_p (OVL_CURRENT (expression)))
-	    return true;
-	  expression = OVL_NEXT (expression);
-	}
+      for (lkp_iterator iter (expression); iter; ++iter)
+	if (type_dependent_expression_p (*iter))
+	  return true;
+
       return false;
     }
 
@@ -24284,12 +24284,9 @@ dependent_template_p (tree tmpl)
 {
   if (TREE_CODE (tmpl) == OVERLOAD)
     {
-      while (tmpl)
-	{
-	  if (dependent_template_p (OVL_CURRENT (tmpl)))
-	    return true;
-	  tmpl = OVL_NEXT (tmpl);
-	}
+      for (lkp_iterator iter (tmpl); iter; ++iter)
+	if (dependent_template_p (*iter))
+	  return true;
       return false;
     }
 
Index: ptree.c
===================================================================
--- ptree.c	(revision 248109)
+++ ptree.c	(working copy)
@@ -236,8 +236,9 @@ cxx_print_xnode (FILE *file, tree node,
 		  indent + 4);
       break;
     case OVERLOAD:
-      print_node (file, "function", OVL_FUNCTION (node), indent+4);
-      print_node (file, "chain", TREE_CHAIN (node), indent+4);
+      print_node (file, "name", OVL_NAME (node), indent+4);
+      for (lkp_iterator iter (node); iter; ++iter)
+	print_node (file, "function", *iter, indent+4);
       break;
     case TEMPLATE_PARM_INDEX:
       print_node (file, "decl", TEMPLATE_PARM_DECL (node), indent+4);
Index: semantics.c
===================================================================
--- semantics.c	(revision 248109)
+++ semantics.c	(working copy)
@@ -5205,26 +5205,33 @@ omp_reduction_lookup (location_t loc, tr
 						  type),
 				false, false);
   tree fns = id;
-  if (id && is_overloaded_fn (id))
-    id = get_fns (id);
-  for (; id; id = OVL_NEXT (id))
+  id = NULL_TREE;
+  if (fns && is_overloaded_fn (fns))
     {
-      tree fndecl = OVL_CURRENT (id);
-      if (TREE_CODE (fndecl) == FUNCTION_DECL)
+      for (lkp_iterator iter (get_fns (fns)); iter; ++iter)
 	{
-	  tree argtype = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
-	  if (same_type_p (TREE_TYPE (argtype), type))
-	    break;
+	  tree fndecl = *iter;
+	  if (TREE_CODE (fndecl) == FUNCTION_DECL)
+	    {
+	      tree argtype = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
+	      if (same_type_p (TREE_TYPE (argtype), type))
+		{
+		  id = fndecl;
+		  break;
+		}
+	    }
+	}
+
+      if (id && BASELINK_P (fns))
+	{
+	  if (baselinkp)
+	    *baselinkp = fns;
+	  else
+	    baselink = fns;
 	}
     }
-  if (id && BASELINK_P (fns))
-    {
-      if (baselinkp)
-	*baselinkp = fns;
-      else
-	baselink = fns;
-    }
-  if (id == NULL_TREE && CLASS_TYPE_P (type) && TYPE_BINFO (type))
+
+  if (!id && CLASS_TYPE_P (type) && TYPE_BINFO (type))
     {
       vec<tree> ambiguous = vNULL;
       tree binfo = TYPE_BINFO (type), base_binfo, ret = NULL_TREE;
@@ -9062,9 +9069,9 @@ classtype_has_nothrow_assign_or_copy_p (
   else
     return false;
 
-  for (; fns; fns = OVL_NEXT (fns))
+  for (ovl_iterator iter (fns); iter; ++iter)
     {
-      tree fn = OVL_CURRENT (fns);
+      tree fn = *iter;
  
       if (assign_p)
 	{
Index: typeck.c
===================================================================
--- typeck.c	(revision 248109)
+++ typeck.c	(working copy)
@@ -2628,23 +2628,20 @@ check_template_keyword (tree decl)
 	permerror (input_location, "%qD is not a template", decl);
       else
 	{
-	  tree fns;
-	  fns = decl;
-	  if (BASELINK_P (fns))
-	    fns = BASELINK_FUNCTIONS (fns);
-	  while (fns)
+	  bool found = false;
+
+	  for (lkp_iterator iter (MAYBE_BASELINK_FUNCTIONS (decl));
+	       !found && iter; ++iter)
 	    {
-	      tree fn = OVL_CURRENT (fns);
+	      tree fn = *iter;
 	      if (TREE_CODE (fn) == TEMPLATE_DECL
-		  || TREE_CODE (fn) == TEMPLATE_ID_EXPR)
-		break;
-	      if (TREE_CODE (fn) == FUNCTION_DECL
-		  && DECL_USE_TEMPLATE (fn)
-		  && PRIMARY_TEMPLATE_P (DECL_TI_TEMPLATE (fn)))
-		break;
-	      fns = OVL_NEXT (fns);
+		  || TREE_CODE (fn) == TEMPLATE_ID_EXPR
+		  || (TREE_CODE (fn) == FUNCTION_DECL
+		      && DECL_USE_TEMPLATE (fn)
+		      && PRIMARY_TEMPLATE_P (DECL_TI_TEMPLATE (fn))))
+		found = true;
 	    }
-	  if (!fns)
+	  if (!found)
 	    permerror (input_location, "%qD is not a template", decl);
 	}
     }

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?

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

[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

[Tim Song]

On Tue, May 16, 2017 at 2:50 PM, Martin Sebor <msebor@gmail.com> 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: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 3:24 PM, Nathan Sidwell <nathan@acm.org> 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: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

[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 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

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 <msebor@gmail.com> 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