[PATCH 0/4] C++ify varobj a little bit

[PATCH 0/4] C++ify varobj a little bit

[Simon Marchi]

This patch series does a basic C++ification of the varobj code, enough
to get rid of the VECs.  It looks regression free according to the
buildbot.

Simon Marchi (4):
  Basic c++ification of varobj
  Make varobj::children an std::vector
  Replace VEC (varobj_update_result) with std::vector
  Remove DEF_VEC_P (varobj_p)

 gdb/ada-varobj.c    |   5 +-
 gdb/mi/mi-cmd-var.c |  62 ++++-----
 gdb/varobj.c        | 362 ++++++++++++++++++----------------------------------
 gdb/varobj.h        |  67 +++++-----
 4 files changed, 189 insertions(+), 307 deletions(-)

-- 
2.15.0

[PATCH 3/4] Replace VEC (varobj_update_result) with std::vector

[Simon Marchi]

This patch replaces makes varobj_update return an std::vector, and
updates the fallouts.

To make that easier, the varobj_update_result is c++ified a bit.  I
added a constructor and initialized its fields in-class.  The newobj
vector is also made an std::vector, so that it's automatically freed
when varobj_update_result is destroyed and handled correctly by the
default move constructor.  I disabled copy constructor and assignment
for that structure, because normally it never needs to be copied, only
moved.

As a result, the newobj parameter of update_dynamic_varobj_children must
be changed to an std::vector.  The patch converts the other vector
parameters of update_dynamic_varobj_children to std::vector.  It's not
strictly necessary to do it in the same patch, but I think it makes
sense to do it.

gdb/ChangeLog:

	* varobj.h (struct varobj_update_result): Add constructor, add
	move constructor, disable copy and assign, initialize fields.
	<newobj>: Change type to std::vector.
	(varobj_update): Return std::vector.
	* varobj.c (install_dynamic_child): Change VEC parameters to
	std::vector and adjust.
	(update_dynamic_varobj_children): Likewise.
	(varobj_update): Return std::vector and adjust.
	* mi/mi-cmd-var.c (varobj_update_one): Adjust to vector changes.
---
 gdb/mi/mi-cmd-var.c |  49 +++++++-----------
 gdb/varobj.c        | 146 ++++++++++++++++++++--------------------------------
 gdb/varobj.h        |  29 +++++++----
 3 files changed, 95 insertions(+), 129 deletions(-)

diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index 084cc38d28..d051874ab2 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -685,27 +685,24 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
 		   int is_explicit)
 {
   struct ui_out *uiout = current_uiout;
-  VEC (varobj_update_result) *changes;
-  varobj_update_result *r;
-  int i;
-  
-  changes = varobj_update (&var, is_explicit);
+
+  std::vector<varobj_update_result> changes = varobj_update (&var, is_explicit);
   
-  for (i = 0; VEC_iterate (varobj_update_result, changes, i, r); ++i)
+  for (const varobj_update_result &r : changes)
     {
       int from, to;
 
       gdb::optional<ui_out_emit_tuple> tuple_emitter;
       if (mi_version (uiout) > 1)
 	tuple_emitter.emplace (uiout, nullptr);
-      uiout->field_string ("name", varobj_get_objname (r->varobj));
+      uiout->field_string ("name", varobj_get_objname (r.varobj));
 
-      switch (r->status)
+      switch (r.status)
 	{
 	case VAROBJ_IN_SCOPE:
-	  if (mi_print_value_p (r->varobj, print_values))
+	  if (mi_print_value_p (r.varobj, print_values))
 	    {
-	      std::string val = varobj_get_value (r->varobj);
+	      std::string val = varobj_get_value (r.varobj);
 
 	      uiout->field_string ("value", val.c_str ());
 	    }
@@ -719,53 +716,47 @@ varobj_update_one (struct varobj *var, enum print_values print_values,
  	  break;
 	}
 
-      if (r->status != VAROBJ_INVALID)
+      if (r.status != VAROBJ_INVALID)
 	{
-	  if (r->type_changed)
+	  if (r.type_changed)
 	    uiout->field_string ("type_changed", "true");
 	  else
 	    uiout->field_string ("type_changed", "false");
 	}
 
-      if (r->type_changed)
+      if (r.type_changed)
 	{
-	  std::string type_name = varobj_get_type (r->varobj);
+	  std::string type_name = varobj_get_type (r.varobj);
 
 	  uiout->field_string ("new_type", type_name.c_str ());
 	}
 
-      if (r->type_changed || r->children_changed)
+      if (r.type_changed || r.children_changed)
 	uiout->field_int ("new_num_children",
-			  varobj_get_num_children (r->varobj));
+			  varobj_get_num_children (r.varobj));
 
       gdb::unique_xmalloc_ptr<char> display_hint
-	= varobj_get_display_hint (r->varobj);
+	= varobj_get_display_hint (r.varobj);
       if (display_hint)
 	uiout->field_string ("displayhint", display_hint.get ());
 
-      if (varobj_is_dynamic_p (r->varobj))
+      if (varobj_is_dynamic_p (r.varobj))
 	uiout->field_int ("dynamic", 1);
 
-      varobj_get_child_range (r->varobj, &from, &to);
-      uiout->field_int ("has_more", varobj_has_more (r->varobj, to));
+      varobj_get_child_range (r.varobj, &from, &to);
+      uiout->field_int ("has_more", varobj_has_more (r.varobj, to));
 
-      if (r->newobj)
+      if (!r.newobj.empty ())
 	{
-	  int j;
-	  varobj_p child;
-
 	  ui_out_emit_list list_emitter (uiout, "new_children");
-	  for (j = 0; VEC_iterate (varobj_p, r->newobj, j, child); ++j)
+
+	  for (varobj *child : r.newobj)
 	    {
 	      ui_out_emit_tuple tuple_emitter (uiout, NULL);
 	      print_varobj (child, print_values, 1 /* print_expression */);
 	    }
-
-	  VEC_free (varobj_p, r->newobj);
-	  r->newobj = NULL;	/* Paranoia.  */
 	}
     }
-  VEC_free (varobj_update_result, changes);
 }
 
 void
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 363ebec570..29c338dd77 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -629,10 +629,10 @@ varobj_restrict_range (const std::vector<varobj *> &children,
 
 static void
 install_dynamic_child (struct varobj *var,
-		       VEC (varobj_p) **changed,
-		       VEC (varobj_p) **type_changed,
-		       VEC (varobj_p) **newobj,
-		       VEC (varobj_p) **unchanged,
+		       std::vector<varobj *> *changed,
+		       std::vector<varobj *> *type_changed,
+		       std::vector<varobj *> *newobj,
+		       std::vector<varobj *> *unchanged,
 		       int *cchanged,
 		       int index,
 		       struct varobj_item *item)
@@ -642,9 +642,9 @@ install_dynamic_child (struct varobj *var,
       /* There's no child yet.  */
       struct varobj *child = varobj_add_child (var, item);
 
-      if (newobj)
+      if (newobj != NULL)
 	{
-	  VEC_safe_push (varobj_p, *newobj, child);
+	  newobj->push_back (child);
 	  *cchanged = 1;
 	}
     }
@@ -655,16 +655,16 @@ install_dynamic_child (struct varobj *var,
 
       if (type_updated)
 	{
-	  if (type_changed)
-	    VEC_safe_push (varobj_p, *type_changed, existing);
+	  if (type_changed != NULL)
+	    type_changed->push_back (existing);
 	}
       if (install_new_value (existing, item->value, 0))
 	{
-	  if (!type_updated && changed)
-	    VEC_safe_push (varobj_p, *changed, existing);
+	  if (!type_updated && changed != NULL)
+	    changed->push_back (existing);
 	}
-      else if (!type_updated && unchanged)
-	VEC_safe_push (varobj_p, *unchanged, existing);
+      else if (!type_updated && unchanged != NULL)
+	unchanged->push_back (existing);
     }
 }
 
@@ -713,10 +713,10 @@ varobj_clear_saved_item (struct varobj_dynamic *var)
 
 static int
 update_dynamic_varobj_children (struct varobj *var,
-				VEC (varobj_p) **changed,
-				VEC (varobj_p) **type_changed,
-				VEC (varobj_p) **newobj,
-				VEC (varobj_p) **unchanged,
+				std::vector<varobj *> *changed,
+				std::vector<varobj *> *type_changed,
+				std::vector<varobj *> *newobj,
+				std::vector<varobj *> *unchanged,
 				int *cchanged,
 				int update_children,
 				int from,
@@ -1525,14 +1525,13 @@ varobj_value_has_mutated (const struct varobj *var, struct value *new_value,
    returns TYPE_CHANGED, then it has done this and VARP will be modified
    to point to the new varobj.  */
 
-VEC(varobj_update_result) *
+std::vector<varobj_update_result>
 varobj_update (struct varobj **varp, int is_explicit)
 {
   int type_changed = 0;
-  int i;
   struct value *newobj;
-  VEC (varobj_update_result) *stack = NULL;
-  VEC (varobj_update_result) *result = NULL;
+  std::vector<varobj_update_result> stack;
+  std::vector<varobj_update_result> result;
 
   /* Frozen means frozen -- we don't check for any change in
      this varobj, including its going out of scope, or
@@ -1544,20 +1543,13 @@ varobj_update (struct varobj **varp, int is_explicit)
 
   if (!(*varp)->root->is_valid)
     {
-      varobj_update_result r = {0};
-
-      r.varobj = *varp;
-      r.status = VAROBJ_INVALID;
-      VEC_safe_push (varobj_update_result, result, &r);
+      result.emplace_back (*varp, VAROBJ_INVALID);
       return result;
     }
 
   if ((*varp)->root->rootvar == *varp)
     {
-      varobj_update_result r = {0};
-
-      r.varobj = *varp;
-      r.status = VAROBJ_IN_SCOPE;
+      varobj_update_result r (*varp);
 
       /* Update the root variable.  value_of_root can return NULL
 	 if the variable is no longer around, i.e. we stepped out of
@@ -1579,28 +1571,23 @@ varobj_update (struct varobj **varp, int is_explicit)
       if (r.status == VAROBJ_NOT_IN_SCOPE)
 	{
 	  if (r.type_changed || r.changed)
-	    VEC_safe_push (varobj_update_result, result, &r);
+	    result.push_back (std::move (r));
+
 	  return result;
 	}
-            
-      VEC_safe_push (varobj_update_result, stack, &r);
-    }
-  else
-    {
-      varobj_update_result r = {0};
 
-      r.varobj = *varp;
-      VEC_safe_push (varobj_update_result, stack, &r);
+      stack.push_back (std::move (r));
     }
+  else
+    stack.emplace_back (*varp);
 
   /* Walk through the children, reconstructing them all.  */
-  while (!VEC_empty (varobj_update_result, stack))
+  while (!stack.empty ())
     {
-      varobj_update_result r = *(VEC_last (varobj_update_result, stack));
+      varobj_update_result r = std::move (stack.back ());
+      stack.pop_back ();
       struct varobj *v = r.varobj;
 
-      VEC_pop (varobj_update_result, stack);
-
       /* Update this variable, unless it's a root, which is already
 	 updated.  */
       if (!r.value_installed)
@@ -1638,9 +1625,8 @@ varobj_update (struct varobj **varp, int is_explicit)
 	 for which -var-list-children was never invoked.  */
       if (varobj_is_dynamic_p (v))
 	{
-	  VEC (varobj_p) *changed = 0, *type_changed = 0, *unchanged = 0;
-	  VEC (varobj_p) *newobj = 0;
-	  int i, children_changed = 0;
+	  std::vector<varobj *> changed, type_changed, unchanged, newobj;
+	  int children_changed = 0;
 
 	  if (v->frozen)
 	    continue;
@@ -1664,7 +1650,7 @@ varobj_update (struct varobj **varp, int is_explicit)
 		}
 
 	      if (r.changed)
-		VEC_safe_push (varobj_update_result, result, &r);
+		result.push_back (std::move (r));
 
 	      continue;
 	    }
@@ -1675,58 +1661,48 @@ varobj_update (struct varobj **varp, int is_explicit)
 					      &unchanged, &children_changed, 1,
 					      v->from, v->to))
 	    {
-	      if (children_changed || newobj)
+	      if (children_changed || !newobj.empty ())
 		{
 		  r.children_changed = 1;
-		  r.newobj = newobj;
+		  r.newobj = std::move (newobj);
 		}
 	      /* Push in reverse order so that the first child is
 		 popped from the work stack first, and so will be
 		 added to result first.  This does not affect
 		 correctness, just "nicer".  */
-	      for (i = VEC_length (varobj_p, type_changed) - 1; i >= 0; --i)
+	      for (int i = type_changed.size () - 1; i >= 0; --i)
 		{
-		  varobj_p tmp = VEC_index (varobj_p, type_changed, i);
-		  varobj_update_result r = {0};
+		  varobj_update_result r (type_changed[i]);
 
 		  /* Type may change only if value was changed.  */
-		  r.varobj = tmp;
 		  r.changed = 1;
 		  r.type_changed = 1;
 		  r.value_installed = 1;
-		  VEC_safe_push (varobj_update_result, stack, &r);
+
+		  stack.push_back (std::move (r));
 		}
-	      for (i = VEC_length (varobj_p, changed) - 1; i >= 0; --i)
+	      for (int i = changed.size () - 1; i >= 0; --i)
 		{
-		  varobj_p tmp = VEC_index (varobj_p, changed, i);
-		  varobj_update_result r = {0};
+		  varobj_update_result r (changed[i]);
 
-		  r.varobj = tmp;
 		  r.changed = 1;
 		  r.value_installed = 1;
-		  VEC_safe_push (varobj_update_result, stack, &r);
+
+		  stack.push_back (std::move (r));
 		}
-	      for (i = VEC_length (varobj_p, unchanged) - 1; i >= 0; --i)
-	      	{
-		  varobj_p tmp = VEC_index (varobj_p, unchanged, i);
-
-	      	  if (!tmp->frozen)
-	      	    {
-	      	      varobj_update_result r = {0};
-
-		      r.varobj = tmp;
-	      	      r.value_installed = 1;
-	      	      VEC_safe_push (varobj_update_result, stack, &r);
-	      	    }
-	      	}
-	      if (r.changed || r.children_changed)
-		VEC_safe_push (varobj_update_result, result, &r);
+	      for (int i = unchanged.size () - 1; i >= 0; --i)
+		{
+		  if (!unchanged[i]->frozen)
+		    {
+		      varobj_update_result r (unchanged[i]);
 
-	      /* Free CHANGED, TYPE_CHANGED and UNCHANGED, but not NEW,
-		 because NEW has been put into the result vector.  */
-	      VEC_free (varobj_p, changed);
-	      VEC_free (varobj_p, type_changed);
-	      VEC_free (varobj_p, unchanged);
+		      r.value_installed = 1;
+
+		      stack.push_back (std::move (r));
+		    }
+		}
+	      if (r.changed || r.children_changed)
+		result.push_back (std::move (r));
 
 	      continue;
 	    }
@@ -1736,29 +1712,21 @@ varobj_update (struct varobj **varp, int is_explicit)
 	 child is popped from the work stack first, and so
 	 will be added to result first.  This does not
 	 affect correctness, just "nicer".  */
-      for (i = v->children.size () - 1; i >= 0; --i)
+      for (int i = v->children.size () - 1; i >= 0; --i)
 	{
 	  varobj *c = v->children[i];
 
 	  /* Child may be NULL if explicitly deleted by -var-delete.  */
 	  if (c != NULL && !c->frozen)
-	    {
-	      varobj_update_result r = {0};
-
-	      r.varobj = c;
-	      VEC_safe_push (varobj_update_result, stack, &r);
-	    }
+	    stack.emplace_back (c);
 	}
 
       if (r.changed || r.type_changed)
-	VEC_safe_push (varobj_update_result, result, &r);
+	result.push_back (std::move (r));
     }
 
-  VEC_free (varobj_update_result, stack);
-
   return result;
 }
-\f
 
 /* Helper functions */
 
diff --git a/gdb/varobj.h b/gdb/varobj.h
index 8384eb53e5..201fac21ab 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -60,26 +60,33 @@ struct varobj;
 typedef struct varobj *varobj_p;
 DEF_VEC_P (varobj_p);
 
-typedef struct varobj_update_result_t
+struct varobj_update_result
 {
+  varobj_update_result (struct varobj *varobj_,
+			varobj_scope_status status_ = VAROBJ_IN_SCOPE)
+  : varobj (varobj_), status (status_)
+  {}
+
+  varobj_update_result (varobj_update_result &&other) = default;
+
+  DISABLE_COPY_AND_ASSIGN (varobj_update_result);
+
   struct varobj *varobj;
-  int type_changed;
-  int children_changed;
-  int changed;
+  int type_changed = 0;
+  int children_changed = 0;
+  int changed = 0;
   enum varobj_scope_status status;
   /* This variable is used internally by varobj_update to indicate if the
      new value of varobj is already computed and installed, or has to
      be yet installed.  Don't use this outside varobj.c.  */
-  int value_installed;  
+  int value_installed = 0;
 
   /* This will be non-NULL when new children were added to the varobj.
      It lists the new children (which must necessarily come at the end
      of the child list) added during an update.  The caller is
      responsible for freeing this vector.  */
-  VEC (varobj_p) *newobj;
-} varobj_update_result;
-
-DEF_VEC_O (varobj_update_result);
+  std::vector<struct varobj *> newobj;
+};
 
 struct varobj_root;
 struct varobj_dynamic;
@@ -305,8 +312,8 @@ extern int varobj_set_value (struct varobj *var, const char *expression);
 extern void all_root_varobjs (void (*func) (struct varobj *var, void *data),
 			      void *data);
 
-extern VEC(varobj_update_result) *varobj_update (struct varobj **varp, 
-						 int is_explicit);
+extern std::vector<varobj_update_result>
+  varobj_update (struct varobj **varp, int is_explicit);
 
 extern void varobj_invalidate (void);
 
-- 
2.15.0

[PATCH 2/4] Make varobj::children an std::vector

[Simon Marchi]

This patch makes the children field of varobj an std::vector, and
updates the fallout.

One note is that varobj::parent must be made non-const.  The reason is
that when a child deletes itself, it modifies its writes NULL to its
slot in its parent's children vector.  With the VEC, the const didn't
made the parent's children vector content const, only the pointer to it,
but with std::vector, even the content is.

gdb/ChangeLog:

	* varobj.h (struct varobj) <parent>: Remove const.
	<children>: Change type to std::vector.
	(varobj_list_children): Return std::vector const reference.
	(varobj_restrict_range): Change parameter type to std::vector
	const reference.
	* varobj.c (varobj_has_more): Adjust.
	(varobj_restrict_range): Change parameter type to std::vector
	const reference and adjust.
	(install_dynamic_child): Adjust.
	(update_dynamic_varobj_children): Adjust.
	(varobj_list_children): Return std::vector const reference and
	adjust.
	(varobj_add_child): Adjust.
	(update_type_if_necessary): Adjust.
	(varobj_update): Adjust.
	(delete_variable_1): Adjust.
	* ada-varobj.c (ada_value_has_mutated): Adjust.
	* mi/mi-cmd-var.c (mi_cmd_var_list_children): Adjust.
---
 gdb/ada-varobj.c    |  5 ++--
 gdb/mi/mi-cmd-var.c | 13 ++++----
 gdb/varobj.c        | 85 +++++++++++++++++++++++++----------------------------
 gdb/varobj.h        | 12 ++++----
 4 files changed, 53 insertions(+), 62 deletions(-)

diff --git a/gdb/ada-varobj.c b/gdb/ada-varobj.c
index 34d9c7b342..8e38c16943 100644
--- a/gdb/ada-varobj.c
+++ b/gdb/ada-varobj.c
@@ -959,7 +959,6 @@ static int
 ada_value_has_mutated (const struct varobj *var, struct value *new_val,
 		       struct type *new_type)
 {
-  int i;
   int from = -1;
   int to = -1;
 
@@ -983,10 +982,10 @@ ada_value_has_mutated (const struct varobj *var, struct value *new_val,
      has mutated or not. So just assume it hasn't.  */
 
   varobj_restrict_range (var->children, &from, &to);
-  for (i = from; i < to; i++)
+  for (int i = from; i < to; i++)
     if (ada_varobj_get_name_of_child (new_val, new_type,
 				      var->name.c_str (), i)
-	!= VEC_index (varobj_p, var->children, i)->name)
+	!= var->children[i]->name)
       return 1;
 
   return 0;
diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index 0215b1a8f5..084cc38d28 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -352,10 +352,7 @@ mi_cmd_var_list_children (const char *command, char **argv, int argc)
 {
   struct ui_out *uiout = current_uiout;
   struct varobj *var;  
-  VEC(varobj_p) *children;
-  struct varobj *child;
   enum print_values print_values;
-  int ix;
   int from, to;
 
   if (argc < 1 || argc > 4)
@@ -379,7 +376,9 @@ mi_cmd_var_list_children (const char *command, char **argv, int argc)
       to = -1;
     }
 
-  children = varobj_list_children (var, &from, &to);
+  const std::vector<varobj *> &children
+    = varobj_list_children (var, &from, &to);
+
   uiout->field_int ("numchild", to - from);
   if (argc == 2 || argc == 4)
     print_values = mi_parse_print_values (argv[0]);
@@ -401,13 +400,11 @@ mi_cmd_var_list_children (const char *command, char **argv, int argc)
 	tuple_emitter.emplace (uiout, "children");
       else
 	list_emitter.emplace (uiout, "children");
-      for (ix = from;
-	   ix < to && VEC_iterate (varobj_p, children, ix, child);
-	   ++ix)
+      for (int ix = from; ix < to && ix < children.size (); ix++)
 	{
 	  ui_out_emit_tuple child_emitter (uiout, "child");
 
-	  print_varobj (child, print_values, 1 /* print expression */);
+	  print_varobj (children[ix], print_values, 1 /* print expression */);
 	}
     }
 
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 2bc7fcd82a..363ebec570 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -558,9 +558,10 @@ varobj_get_display_hint (const struct varobj *var)
 int
 varobj_has_more (const struct varobj *var, int to)
 {
-  if (VEC_length (varobj_p, var->children) > to)
+  if (var->children.size () > to)
     return 1;
-  return ((to == -1 || VEC_length (varobj_p, var->children) == to)
+
+  return ((to == -1 || var->children.size () == to)
 	  && (var->dynamic->saved_item != NULL));
 }
 
@@ -602,19 +603,22 @@ varobj_get_frozen (const struct varobj *var)
    used.  */
 
 void
-varobj_restrict_range (VEC (varobj_p) *children, int *from, int *to)
+varobj_restrict_range (const std::vector<varobj *> &children,
+		       int *from, int *to)
 {
+  int len = children.size ();
+
   if (*from < 0 || *to < 0)
     {
       *from = 0;
-      *to = VEC_length (varobj_p, children);
+      *to = len;
     }
   else
     {
-      if (*from > VEC_length (varobj_p, children))
-	*from = VEC_length (varobj_p, children);
-      if (*to > VEC_length (varobj_p, children))
-	*to = VEC_length (varobj_p, children);
+      if (*from > len)
+	*from = len;
+      if (*to > len)
+	*to = len;
       if (*from > *to)
 	*from = *to;
     }
@@ -633,7 +637,7 @@ install_dynamic_child (struct varobj *var,
 		       int index,
 		       struct varobj_item *item)
 {
-  if (VEC_length (varobj_p, var->children) < index + 1)
+  if (var->children.size () < index + 1)
     {
       /* There's no child yet.  */
       struct varobj *child = varobj_add_child (var, item);
@@ -646,7 +650,7 @@ install_dynamic_child (struct varobj *var,
     }
   else
     {
-      varobj_p existing = VEC_index (varobj_p, var->children, index);
+      varobj *existing = var->children[index];
       int type_updated = update_type_if_necessary (existing, item->value);
 
       if (type_updated)
@@ -735,7 +739,7 @@ update_dynamic_varobj_children (struct varobj *var,
 	return 0;
     }
   else
-    i = VEC_length (varobj_p, var->children);
+    i = var->children.size ();
 
   /* We ask for one extra child, so that MI can report whether there
      are more children.  */
@@ -789,22 +793,21 @@ update_dynamic_varobj_children (struct varobj *var,
 	}
     }
 
-  if (i < VEC_length (varobj_p, var->children))
+  if (i < var->children.size ())
     {
-      int j;
-
       *cchanged = 1;
-      for (j = i; j < VEC_length (varobj_p, var->children); ++j)
-	varobj_delete (VEC_index (varobj_p, var->children, j), 0);
-      VEC_truncate (varobj_p, var->children, i);
+      for (int j = i; j < var->children.size (); ++j)
+	varobj_delete (var->children[j], 0);
+
+      var->children.resize (i);
     }
 
   /* If there are fewer children than requested, note that the list of
      children changed.  */
-  if (to >= 0 && VEC_length (varobj_p, var->children) < to)
+  if (to >= 0 && var->children.size () < to)
     *cchanged = 1;
 
-  var->num_children = VEC_length (varobj_p, var->children);
+  var->num_children = var->children.size ();
 
   return 1;
 }
@@ -833,10 +836,10 @@ varobj_get_num_children (struct varobj *var)
 /* Creates a list of the immediate children of a variable object;
    the return code is the number of such children or -1 on error.  */
 
-VEC (varobj_p)*
+const std::vector<varobj *> &
 varobj_list_children (struct varobj *var, int *from, int *to)
 {
-  int i, children_changed;
+  int children_changed;
 
   var->dynamic->children_requested = 1;
 
@@ -860,21 +863,18 @@ varobj_list_children (struct varobj *var, int *from, int *to)
 
   /* If we're called when the list of children is not yet initialized,
      allocate enough elements in it.  */
-  while (VEC_length (varobj_p, var->children) < var->num_children)
-    VEC_safe_push (varobj_p, var->children, NULL);
+  while (var->children.size () < var->num_children)
+    var->children.push_back (NULL);
 
-  for (i = 0; i < var->num_children; i++)
+  for (int i = 0; i < var->num_children; i++)
     {
-      varobj_p existing = VEC_index (varobj_p, var->children, i);
-
-      if (existing == NULL)
+      if (var->children[i] == NULL)
 	{
 	  /* Either it's the first call to varobj_list_children for
 	     this variable object, and the child was never created,
 	     or it was explicitly deleted by the client.  */
 	  std::string name = name_of_child (var, i);
-	  existing = create_child (var, i, name);
-	  VEC_replace (varobj_p, var->children, i, existing);
+	  var->children[i] = create_child (var, i, name);
 	}
     }
 
@@ -885,11 +885,10 @@ varobj_list_children (struct varobj *var, int *from, int *to)
 static struct varobj *
 varobj_add_child (struct varobj *var, struct varobj_item *item)
 {
-  varobj_p v = create_child_with_value (var,
-					VEC_length (varobj_p, var->children), 
-					item);
+  varobj *v = create_child_with_value (var, var->children.size (), item);
+
+  var->children.push_back (v);
 
-  VEC_safe_push (varobj_p, var->children, v);
   return v;
 }
 
@@ -1221,7 +1220,7 @@ update_type_if_necessary (struct varobj *var, struct value *new_value)
 
 	      /* This information may be not valid for a new type.  */
 	      varobj_delete (var, 1);
-	      VEC_free (varobj_p, var->children);
+	      var->children.clear ();
 	      var->num_children = -1;
 	      return 1;
 	    }
@@ -1737,9 +1736,9 @@ varobj_update (struct varobj **varp, int is_explicit)
 	 child is popped from the work stack first, and so
 	 will be added to result first.  This does not
 	 affect correctness, just "nicer".  */
-      for (i = VEC_length (varobj_p, v->children)-1; i >= 0; --i)
+      for (i = v->children.size () - 1; i >= 0; --i)
 	{
-	  varobj_p c = VEC_index (varobj_p, v->children, i);
+	  varobj *c = v->children[i];
 
 	  /* Child may be NULL if explicitly deleted by -var-delete.  */
 	  if (c != NULL && !c->frozen)
@@ -1786,20 +1785,18 @@ static void
 delete_variable_1 (int *delcountp, struct varobj *var, int only_children_p,
 		   int remove_from_parent_p)
 {
-  int i;
-
   /* Delete any children of this variable, too.  */
-  for (i = 0; i < VEC_length (varobj_p, var->children); ++i)
+  for (varobj *child : var->children)
     {   
-      varobj_p child = VEC_index (varobj_p, var->children, i);
-
       if (!child)
 	continue;
+
       if (!remove_from_parent_p)
 	child->parent = NULL;
+
       delete_variable_1 (delcountp, child, 0, only_children_p);
     }
-  VEC_free (varobj_p, var->children);
+  var->children.clear ();
 
   /* if we were called to delete only the children we are done here.  */
   if (only_children_p)
@@ -1819,9 +1816,7 @@ delete_variable_1 (int *delcountp, struct varobj *var, int only_children_p,
      expensive list search to find the element to remove when we are
      discarding the list afterwards.  */
   if ((remove_from_parent_p) && (var->parent != NULL))
-    {
-      VEC_replace (varobj_p, var->parent->children, var->index, NULL);
-    }
+    var->parent->children[var->index] = NULL;
 
   if (!var->obj_name.empty ())
     uninstall_variable (var);
diff --git a/gdb/varobj.h b/gdb/varobj.h
index d09b99b7f2..8384eb53e5 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -124,10 +124,10 @@ struct varobj
   int num_children = -1;
 
   /* If this object is a child, this points to its immediate parent.  */
-  const struct varobj *parent = NULL;
+  struct varobj *parent = NULL;
 
   /* Children of this object.  */
-  VEC (varobj_p) *children = NULL;
+  std::vector<varobj *> children;
 
   /* Description of the root variable.  Points to root variable for
      children.  */
@@ -280,8 +280,8 @@ extern int varobj_get_num_children (struct varobj *var);
    that was returned.  The resulting VEC will contain at least the
    children from *FROM to just before *TO; it might contain more
    children, depending on whether any more were available.  */
-extern VEC (varobj_p)* varobj_list_children (struct varobj *var,
-					     int *from, int *to);
+extern const std::vector<varobj *> &
+  varobj_list_children (struct varobj *var, int *from, int *to);
 
 extern std::string varobj_get_type (struct varobj *var);
 
@@ -341,8 +341,8 @@ extern std::string
 extern void varobj_formatted_print_options (struct value_print_options *opts,
 					    enum varobj_display_formats format);
 
-extern void varobj_restrict_range (VEC (varobj_p) *children, int *from,
-				   int *to);
+extern void varobj_restrict_range (const std::vector<varobj *> &children,
+				   int *from, int *to);
 
 extern int varobj_default_is_path_expr_parent (const struct varobj *var);
 
-- 
2.15.0

[PATCH 4/4] Remove DEF_VEC_P (varobj_p)

[Simon Marchi]

The last patch removed the last usage of this type, so we can remove it.

gdb/ChangeLog:

	* varobj.h (DEF_VEC_P (varobj_p)): Remove.
---
 gdb/varobj.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/gdb/varobj.h b/gdb/varobj.h
index 201fac21ab..bba7a55248 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -57,9 +57,6 @@ extern const char *varobj_format_string[];
 
 struct varobj;
 
-typedef struct varobj *varobj_p;
-DEF_VEC_P (varobj_p);
-
 struct varobj_update_result
 {
   varobj_update_result (struct varobj *varobj_,
-- 
2.15.0

[PATCH 1/4] Basic c++ification of varobj

[Simon Marchi]

This patch does a basic c++ification or the varobj data structure.

  - varobj: add constructor and destructor, initialize fields
  - varobj_root: initialize fields
  - varobj_dynamic: initialize fields

This allows getting rid of new_variable, new_root_variable.
free_variable essentially becomes varobj's destructor.  This also allows
getting rid of a cleanup, make_cleanup_free_variable, which was only
used in varobj_create in case the varobj creation fails.  It is replaced
with a unique_ptr.

varobj and varobj_root were already allocated/deallocated with
new/delete.  varobj_dynamic wasn't, now it is.

gdb/ChangeLog:

	* varobj.h (struct varobj): Add constructor and destructor,
	initialize fields.
	* varobj.c (struct varobj_root): Initialize fields.
	(struct varobj_dynamic): Initialize fields.
	(varobj_create): Use unique_ptr instead of cleanup.  Create
	varobj with new instead of new_root_variable.
	(delete_variable_1): Free variable with delete instead of
	free_variable.
	(create_child_with_value): Create variable with new instead of
	new_variable.
	(varobj::varobj): New.
	(varobj::~varobj): New (body mostly coming from free_variable).
	(new_variable): Remove.
	(free_variable): Remove.
	(do_free_variable_cleanup): Remove.
	(make_cleanup_free_variable): Remove.

---
 gdb/varobj.c | 133 +++++++++++++----------------------------------------------
 gdb/varobj.h |  27 ++++++------
 2 files changed, 44 insertions(+), 116 deletions(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 2d850fb5e1..2bc7fcd82a 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -68,42 +68,41 @@ varobj_enable_pretty_printing (void)
    varobj.  */
 struct varobj_root
 {
-
   /* The expression for this parent.  */
   expression_up exp;
 
   /* Block for which this expression is valid.  */
-  const struct block *valid_block;
+  const struct block *valid_block = NULL;
 
   /* The frame for this expression.  This field is set iff valid_block is
      not NULL.  */
-  struct frame_id frame;
+  struct frame_id frame = null_frame_id;
 
   /* The global thread ID that this varobj_root belongs to.  This field
      is only valid if valid_block is not NULL.
      When not 0, indicates which thread 'frame' belongs to.
      When 0, indicates that the thread list was empty when the varobj_root
      was created.  */
-  int thread_id;
+  int thread_id = 0;
 
   /* If 1, the -var-update always recomputes the value in the
      current thread and frame.  Otherwise, variable object is
      always updated in the specific scope/thread/frame.  */
-  int floating;
+  int floating = 0;
 
   /* Flag that indicates validity: set to 0 when this varobj_root refers 
      to symbols that do not exist anymore.  */
-  int is_valid;
+  int is_valid = 1;
 
   /* Language-related operations for this variable and its
      children.  */
-  const struct lang_varobj_ops *lang_ops;
+  const struct lang_varobj_ops *lang_ops = NULL;
 
   /* The varobj for this root node.  */
-  struct varobj *rootvar;
+  struct varobj *rootvar = NULL;
 
   /* Next root variable */
-  struct varobj_root *next;
+  struct varobj_root *next = NULL;
 };
 
 /* Dynamic part of varobj.  */
@@ -114,27 +113,27 @@ struct varobj_dynamic
      used to decide if dynamic varobj should recompute their children.
      In the event that the frontend never asked for the children, we
      can avoid that.  */
-  int children_requested;
+  int children_requested = 0;
 
   /* The pretty-printer constructor.  If NULL, then the default
      pretty-printer will be looked up.  If None, then no
      pretty-printer will be installed.  */
-  PyObject *constructor;
+  PyObject *constructor = NULL;
 
   /* The pretty-printer that has been constructed.  If NULL, then a
      new printer object is needed, and one will be constructed.  */
-  PyObject *pretty_printer;
+  PyObject *pretty_printer = NULL;
 
   /* The iterator returned by the printer's 'children' method, or NULL
      if not available.  */
-  struct varobj_iter *child_iter;
+  struct varobj_iter *child_iter = NULL;
 
   /* We request one extra item from the iterator, so that we can
      report to the caller whether there are more items than we have
      already reported.  However, we don't want to install this value
      when we read it, because that will mess up future updates.  So,
      we stash it here instead.  */
-  varobj_item *saved_item;
+  varobj_item *saved_item = NULL;
 };
 
 /* A list of varobjs */
@@ -165,14 +164,6 @@ create_child_with_value (struct varobj *parent, int index,
 
 /* Utility routines */
 
-static struct varobj *new_variable (void);
-
-static struct varobj *new_root_variable (void);
-
-static void free_variable (struct varobj *var);
-
-static struct cleanup *make_cleanup_free_variable (struct varobj *var);
-
 static enum varobj_display_formats variable_default_display (struct varobj *);
 
 static int update_type_if_necessary (struct varobj *var,
@@ -273,12 +264,8 @@ struct varobj *
 varobj_create (const char *objname,
 	       const char *expression, CORE_ADDR frame, enum varobj_type type)
 {
-  struct varobj *var;
-  struct cleanup *old_chain;
-
   /* Fill out a varobj structure for the (root) variable being constructed.  */
-  var = new_root_variable ();
-  old_chain = make_cleanup_free_variable (var);
+  std::unique_ptr<varobj> var (new varobj (new varobj_root));
 
   if (expression != NULL)
     {
@@ -333,7 +320,6 @@ varobj_create (const char *objname,
 
       CATCH (except, RETURN_MASK_ERROR)
 	{
-	  do_cleanups (old_chain);
 	  return NULL;
 	}
       END_CATCH
@@ -343,13 +329,12 @@ varobj_create (const char *objname,
 	  || var->root->exp->elts[0].opcode == OP_TYPEOF
 	  || var->root->exp->elts[0].opcode == OP_DECLTYPE)
 	{
-	  do_cleanups (old_chain);
 	  fprintf_unfiltered (gdb_stderr, "Attempt to use a type name"
 			      " as an expression.\n");
 	  return NULL;
 	}
 
-      var->format = variable_default_display (var);
+      var->format = variable_default_display (var.get ());
       var->root->valid_block = innermost_block;
       var->name = expression;
       /* For a root var, the name and the expr are the same.  */
@@ -403,10 +388,10 @@ varobj_create (const char *objname,
       /* Set language info */
       var->root->lang_ops = var->root->exp->language_defn->la_varobj_ops;
 
-      install_new_value (var, value, 1 /* Initial assignment */);
+      install_new_value (var.get (), value, 1 /* Initial assignment */);
 
       /* Set ourselves as our root.  */
-      var->root->rootvar = var;
+      var->root->rootvar = var.get ();
 
       /* Reset the selected frame.  */
       if (frame_id_p (old_id))
@@ -422,15 +407,11 @@ varobj_create (const char *objname,
 
       /* If a varobj name is duplicated, the install will fail so
          we must cleanup.  */
-      if (!install_variable (var))
-	{
-	  do_cleanups (old_chain);
-	  return NULL;
-	}
+      if (!install_variable (var.get ()))
+	return NULL;
     }
 
-  discard_cleanups (old_chain);
-  return var;
+  return var.release ();
 }
 
 /* Generates an unique name that can be used for a varobj.  */
@@ -1846,7 +1827,7 @@ delete_variable_1 (int *delcountp, struct varobj *var, int only_children_p,
     uninstall_variable (var);
 
   /* Free memory associated with this variable.  */
-  free_variable (var);
+  delete var;
 }
 
 /* Install the given variable VAR with the object name VAR->OBJ_NAME.  */
@@ -1985,15 +1966,12 @@ static struct varobj *
 create_child_with_value (struct varobj *parent, int index,
 			 struct varobj_item *item)
 {
-  struct varobj *child;
-
-  child = new_variable ();
+  varobj *child = new varobj (parent->root);
 
   /* NAME is allocated by caller.  */
   std::swap (child->name, item->name);
   child->index = index;
   child->parent = parent;
-  child->root = parent->root;
 
   if (varobj_is_anonymous_child (child))
     child->obj_name = string_printf ("%s.%d_anonymous",
@@ -2026,57 +2004,17 @@ create_child_with_value (struct varobj *parent, int index,
  */
 
 /* Allocate memory and initialize a new variable.  */
-static struct varobj *
-new_variable (void)
+varobj::varobj (varobj_root *root_)
+: root (root_), dynamic (new varobj_dynamic)
 {
-  struct varobj *var;
-
-  var = new varobj ();
-  var->index = -1;
-  var->type = NULL;
-  var->value = NULL;
-  var->num_children = -1;
-  var->parent = NULL;
-  var->children = NULL;
-  var->format = FORMAT_NATURAL;
-  var->root = NULL;
-  var->updated = 0;
-  var->frozen = 0;
-  var->not_fetched = 0;
-  var->dynamic = XNEW (struct varobj_dynamic);
-  var->dynamic->children_requested = 0;
-  var->from = -1;
-  var->to = -1;
-  var->dynamic->constructor = 0;
-  var->dynamic->pretty_printer = 0;
-  var->dynamic->child_iter = 0;
-  var->dynamic->saved_item = 0;
-
-  return var;
-}
-
-/* Allocate memory and initialize a new root variable.  */
-static struct varobj *
-new_root_variable (void)
-{
-  struct varobj *var = new_variable ();
-
-  var->root = new varobj_root ();
-  var->root->lang_ops = NULL;
-  var->root->exp = NULL;
-  var->root->valid_block = NULL;
-  var->root->frame = null_frame_id;
-  var->root->floating = 0;
-  var->root->rootvar = NULL;
-  var->root->is_valid = 1;
-
-  return var;
 }
 
 /* Free any allocated memory associated with VAR.  */
-static void
-free_variable (struct varobj *var)
+
+varobj::~varobj ()
 {
+  varobj *var = this;
+
 #if HAVE_PYTHON
   if (var->dynamic->pretty_printer != NULL)
     {
@@ -2094,20 +2032,7 @@ free_variable (struct varobj *var)
   if (is_root_p (var))
     delete var->root;
 
-  xfree (var->dynamic);
-  delete var;
-}
-
-static void
-do_free_variable_cleanup (void *var)
-{
-  free_variable ((struct varobj *) var);
-}
-
-static struct cleanup *
-make_cleanup_free_variable (struct varobj *var)
-{
-  return make_cleanup (do_free_variable_cleanup, var);
+  delete var->dynamic;
 }
 
 /* Return the type of the value that's stored in VAR,
diff --git a/gdb/varobj.h b/gdb/varobj.h
index 0d4a537d7e..d09b99b7f2 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -89,6 +89,9 @@ struct varobj_dynamic;
    a particular object variable.  */
 struct varobj
 {
+  varobj (varobj_root *root_);
+  ~varobj ();
+
   /* Name of the variable for this object.  If this variable is a
      child, then this name will be the child's source name.
      (bar, not foo.bar).  */
@@ -104,37 +107,37 @@ struct varobj
   std::string obj_name;
 
   /* Index of this variable in its parent or -1.  */
-  int index;
+  int index = -1;
 
   /* The type of this variable.  This can be NULL
      for artificial variable objects -- currently, the "accessibility"
      variable objects in C++.  */
-  struct type *type;
+  struct type *type = NULL;
 
   /* The value of this expression or subexpression.  A NULL value
      indicates there was an error getting this value.
      Invariant: if varobj_value_is_changeable_p (this) is non-zero, 
      the value is either NULL, or not lazy.  */
-  struct value *value;
+  struct value *value = NULL;
 
   /* The number of (immediate) children this variable has.  */
-  int num_children;
+  int num_children = -1;
 
   /* If this object is a child, this points to its immediate parent.  */
-  const struct varobj *parent;
+  const struct varobj *parent = NULL;
 
   /* Children of this object.  */
-  VEC (varobj_p) *children;
+  VEC (varobj_p) *children = NULL;
 
   /* Description of the root variable.  Points to root variable for
      children.  */
   struct varobj_root *root;
 
   /* The format of the output for this object.  */
-  enum varobj_display_formats format;
+  enum varobj_display_formats format = FORMAT_NATURAL;
 
   /* Was this variable updated via a varobj_set_value operation.  */
-  int updated;
+  int updated = 0;
 
   /* Last print value.  */
   std::string print_value;
@@ -142,18 +145,18 @@ struct varobj
   /* Is this variable frozen.  Frozen variables are never implicitly
      updated by -var-update * 
      or -var-update <direct-or-indirect-parent>.  */
-  int frozen;
+  int frozen = 0;
 
   /* Is the value of this variable intentionally not fetched?  It is
      not fetched if either the variable is frozen, or any parents is
      frozen.  */
-  int not_fetched;
+  int not_fetched = 0;
 
   /* Sub-range of children which the MI consumer has requested.  If
      FROM < 0 or TO < 0, means that all children have been
      requested.  */
-  int from;
-  int to;
+  int from = -1;
+  int to = -1;
 
   /* Dynamic part of varobj.  */
   struct varobj_dynamic *dynamic;
-- 
2.15.0

Re: [PATCH 1/4] Basic c++ification of varobj

[Yao Qi]

On 17-11-18 16:46:03, Simon Marchi wrote:
>  
>    /* If 1, the -var-update always recomputes the value in the
>       current thread and frame.  Otherwise, variable object is
>       always updated in the specific scope/thread/frame.  */
> -  int floating;
> +  int floating = 0;
>  
>    /* Flag that indicates validity: set to 0 when this varobj_root refers 
>       to symbols that do not exist anymore.  */
> -  int is_valid;
> +  int is_valid = 1;

Since we do the change, can we change these fields to bool?

> @@ -114,27 +113,27 @@ struct varobj_dynamic
>       used to decide if dynamic varobj should recompute their children.
>       In the event that the frontend never asked for the children, we
>       can avoid that.  */
> -  int children_requested;
> +  int children_requested = 0;
> 

Likewise.  Otherwise, it is good to me.

-- 
Yao

Re: [PATCH 1/4] Basic c++ification of varobj

[Simon Marchi]

On 2017-11-21 16:30, Yao Qi wrote:
> On 17-11-18 16:46:03, Simon Marchi wrote:
>> 
>>    /* If 1, the -var-update always recomputes the value in the
>>       current thread and frame.  Otherwise, variable object is
>>       always updated in the specific scope/thread/frame.  */
>> -  int floating;
>> +  int floating = 0;
>> 
>>    /* Flag that indicates validity: set to 0 when this varobj_root 
>> refers
>>       to symbols that do not exist anymore.  */
>> -  int is_valid;
>> +  int is_valid = 1;
> 
> Since we do the change, can we change these fields to bool?

To keep patches simple, I'll do that in another patch on top of that 
series.  There are more places we can change variables to bool, so I'll 
make a pass on the whole varobj.c/h.

>> @@ -114,27 +113,27 @@ struct varobj_dynamic
>>       used to decide if dynamic varobj should recompute their 
>> children.
>>       In the event that the frontend never asked for the children, we
>>       can avoid that.  */
>> -  int children_requested;
>> +  int children_requested = 0;
>> 
> 
> Likewise.  Otherwise, it is good to me.

Thanks!

Simon

Re: [PATCH 1/4] Basic c++ification of varobj

[Pedro Alves]

On 11/18/2017 09:46 PM, Simon Marchi wrote:

>  /* Return the type of the value that's stored in VAR,
> diff --git a/gdb/varobj.h b/gdb/varobj.h
> index 0d4a537d7e..d09b99b7f2 100644
> --- a/gdb/varobj.h
> +++ b/gdb/varobj.h
> @@ -89,6 +89,9 @@ struct varobj_dynamic;
>     a particular object variable.  */
>  struct varobj
>  {
> +  varobj (varobj_root *root_);

explicit?

Thanks,
Pedro Alves

Re: [PATCH 1/4] Basic c++ification of varobj

[Simon Marchi]

On 2017-11-22 05:44, Pedro Alves wrote:
> On 11/18/2017 09:46 PM, Simon Marchi wrote:
> 
>>  /* Return the type of the value that's stored in VAR,
>> diff --git a/gdb/varobj.h b/gdb/varobj.h
>> index 0d4a537d7e..d09b99b7f2 100644
>> --- a/gdb/varobj.h
>> +++ b/gdb/varobj.h
>> @@ -89,6 +89,9 @@ struct varobj_dynamic;
>>     a particular object variable.  */
>>  struct varobj
>>  {
>> +  varobj (varobj_root *root_);
> 
> explicit?

Good idea, fixed locally.

Re: [PATCH 0/4] C++ify varobj a little bit

[Yao Qi]

On 17-11-18 16:46:02, Simon Marchi wrote:
> This patch series does a basic C++ification of the varobj code, enough
> to get rid of the VECs.  It looks regression free according to the
> buildbot.

I go through them quickly, and they look good to me.

-- 
Yao

Re: [PATCH 0/4] C++ify varobj a little bit

[Simon Marchi]

On 2017-11-21 16:36, Yao Qi wrote:
> On 17-11-18 16:46:02, Simon Marchi wrote:
>> This patch series does a basic C++ification of the varobj code, enough
>> to get rid of the VECs.  It looks regression free according to the
>> buildbot.
> 
> I go through them quickly, and they look good to me.

Thanks, I pushed the series, including the "explicit" suggested by 
Pedro.

Simon