[PATCH 1/6] Free results of varobj_get_expression

[PATCH 1/6] Free results of varobj_get_expression

[Simon Marchi]

varobj_get_expression returns an allocated string, which must be freed
by the caller.

gdb/ChangeLog:

	* mi-cmd-var.c (print_varobj): Free varobj_get_expression
	result.
	(mi_cmd_var_info_expression): Same.
	* varobj.c (varobj_get_expression): Mention in the comment that
	the result must by freed by the caller.
---
 gdb/mi/mi-cmd-var.c | 13 +++++++++++--
 gdb/varobj.c        |  3 ++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index d873a17..d9b37f8 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -55,7 +55,12 @@ print_varobj (struct varobj *var, enum print_values print_values,
 
   ui_out_field_string (uiout, "name", varobj_get_objname (var));
   if (print_expression)
-    ui_out_field_string (uiout, "exp", varobj_get_expression (var));
+    {
+      char *exp = varobj_get_expression (var);
+
+      ui_out_field_string (uiout, "exp", exp);
+      xfree (exp);
+    }
   ui_out_field_int (uiout, "numchild", varobj_get_num_children (var));
   
   if (mi_print_value_p (var, print_values))
@@ -485,6 +490,7 @@ mi_cmd_var_info_expression (char *command, char **argv, int argc)
   struct ui_out *uiout = current_uiout;
   const struct language_defn *lang;
   struct varobj *var;
+  char *exp;
 
   if (argc != 1)
     error (_("-var-info-expression: Usage: NAME."));
@@ -495,7 +501,10 @@ mi_cmd_var_info_expression (char *command, char **argv, int argc)
   lang = varobj_get_language (var);
 
   ui_out_field_string (uiout, "lang", lang->la_natural_name);
-  ui_out_field_string (uiout, "exp", varobj_get_expression (var));
+
+  exp = varobj_get_expression (var);
+  ui_out_field_string (uiout, "exp", exp);
+  xfree (exp);
 }
 
 void
diff --git a/gdb/varobj.c b/gdb/varobj.c
index dad284d..6c9257d 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -492,7 +492,8 @@ varobj_get_objname (struct varobj *var)
   return var->obj_name;
 }
 
-/* Given the handle, return the expression represented by the object.  */
+/* Given the handle, return the expression represented by the object.  The
+   result must be freed by the caller.  */
 
 char *
 varobj_get_expression (struct varobj *var)
-- 
2.1.4

[PATCH 3/6] Set varobj->path_expr in varobj_get_path_expr

[Simon Marchi]

It seems like different languages are doing this differently (e.g.
C and Ada). For C, var->path_expr is set inside c_path_expr_of_child.
The next time the value is requested, is it therefore not recomputed.
Ada does not set this field, but just returns the value. Since the field
is never set, the value is recomputed every time it is requested.

This patch makes it so that path_expr_of_child's only job is to compute
the path expression, not save/cache the value. The field is set by the
varobj common code.

gdb/ChangeLog:

	* varobj.c (varobj_get_path_expr): Set var->path_expr.
	* c-varobj.c (c_path_expr_of_child): Set local var instead of
	child->path_expr.
	(cplus_path_expr_of_child): Same.
---
 gdb/c-varobj.c | 12 ++++++++----
 gdb/varobj.c   |  9 +++++----
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
index 1db0957..bd0e5fb 100644
--- a/gdb/c-varobj.c
+++ b/gdb/c-varobj.c
@@ -433,9 +433,11 @@ c_name_of_child (struct varobj *parent, int index)
 static char *
 c_path_expr_of_child (struct varobj *child)
 {
+  char *path_expr;
+
   c_describe_child (child->parent, child->index, NULL, NULL, NULL, 
-		    &child->path_expr);
-  return child->path_expr;
+		    &path_expr);
+  return path_expr;
 }
 
 static struct value *
@@ -906,9 +908,11 @@ cplus_name_of_child (struct varobj *parent, int index)
 static char *
 cplus_path_expr_of_child (struct varobj *child)
 {
+  char *path_expr;
+
   cplus_describe_child (child->parent, child->index, NULL, NULL, NULL, 
-			&child->path_expr);
-  return child->path_expr;
+			&path_expr);
+  return path_expr;
 }
 
 static struct value *
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 6c9257d..28d388e 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1034,16 +1034,17 @@ varobj_get_path_expr_parent (struct varobj *var)
 char *
 varobj_get_path_expr (struct varobj *var)
 {
-  if (var->path_expr != NULL)
-    return var->path_expr;
-  else 
+  if (var->path_expr == NULL)
     {
       /* For root varobjs, we initialize path_expr
 	 when creating varobj, so here it should be
 	 child varobj.  */
       gdb_assert (!is_root_p (var));
-      return (*var->root->lang_ops->path_expr_of_child) (var);
+
+      var->path_expr = (*var->root->lang_ops->path_expr_of_child) (var);
     }
+
+  return var->path_expr;
 }
 
 const struct language_defn *
-- 
2.1.4

[PATCH 2/6] Mention which return values need to be freed in lang_varobj_ops

[Simon Marchi]

This is the result of a little bit of investigation of the C and Ada
languages, as well as some common sense.

gdb/ChangeLog:

	* varobj.h (lang_varobj_ops): Mention which return values need
	to be freed.
---
 gdb/varobj.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/gdb/varobj.h b/gdb/varobj.h
index 796b940..145b963 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -169,23 +169,28 @@ struct lang_varobj_ops
   /* The number of children of PARENT.  */
   int (*number_of_children) (struct varobj *parent);
 
-  /* The name (expression) of a root varobj.  */
+  /* The name (expression) of a root varobj.  The returned value must be freed
+     by the caller.  */
   char *(*name_of_variable) (struct varobj *parent);
 
-  /* The name of the INDEX'th child of PARENT.  */
+  /* The name of the INDEX'th child of PARENT.  The returned value must be
+     freed by the caller.  */
   char *(*name_of_child) (struct varobj *parent, int index);
 
   /* Returns the rooted expression of CHILD, which is a variable
-     obtain that has some parent.  */
+     obtain that has some parent.  The returned value must be freed by the
+     caller.  */
   char *(*path_expr_of_child) (struct varobj *child);
 
-  /* The ``struct value *'' of the INDEX'th child of PARENT.  */
+  /* The ``struct value *'' of the INDEX'th child of PARENT.  The returned
+     value must be freed by the caller.  */
   struct value *(*value_of_child) (struct varobj *parent, int index);
 
   /* The type of the INDEX'th child of PARENT.  */
   struct type *(*type_of_child) (struct varobj *parent, int index);
 
-  /* The current value of VAR.  */
+  /* The current value of VAR.  The returned value must be freed by the
+     caller.  */
   char *(*value_of_variable) (struct varobj *var,
 			      enum varobj_display_formats format);
 
-- 
2.1.4

[PATCH 6/6] Fix varobj_delete comment

[Simon Marchi]

gdb/ChangeLog:

	* varobj.c (varobj_delete): Fix comment.
---
 gdb/varobj.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index d3fa1ba..98e9e43 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -502,9 +502,10 @@ varobj_get_expression (const struct varobj *var)
 }
 
 /* Deletes a varobj and all its children if only_children == 0,
-   otherwise deletes only the children; returns a malloc'ed list of
-   all the (malloc'ed) names of the variables that have been deleted
-   (NULL terminated).  */
+   otherwise deletes only the children. If DELLIST is non-NULL, it is
+   assigned a malloc'ed list of all the (malloc'ed) names of the variables
+   that have been deleted (NULL terminated).  Returns the number of deleted
+   variables.  */
 
 int
 varobj_delete (struct varobj *var, char ***dellist, int only_children)
-- 
2.1.4

[PATCH 5/6] Mention that create_child takes ownership of the allocated name

[Simon Marchi]

gdb/ChangeLog:

	* varobj.c (create_child): Modify comment.
---
 gdb/varobj.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 0daef91..d3fa1ba 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2043,7 +2043,9 @@ uninstall_variable (struct varobj *var)
 
 }
 
-/* Create and install a child of the parent of the given name.  */
+/* Create and install a child of the parent of the given name.
+
+   The created VAROBJ takes ownership of the allocated NAME.  */
 static struct varobj *
 create_child (struct varobj *parent, int index, char *name)
 {
-- 
2.1.4

[PATCH 4/6] Constify some parameters in the varobj code

[Simon Marchi]

To make it clear that some functions should not modify the variable
object, this patch adds the const qualifier where it makes sense to some
struct varobj * parameters. Most getters should take a const pointer to
guarantee they don't modify the object.

Unfortunately, I couldn't add it to some callbacks (such as name_of_child).
In the C implementation, they call c_describe_child, which calls
varobj_get_path_expr. varobj_get_path_expr needs to modify the object in
order to cache the computed value. It therefore can't take a const
pointer, and it affects the whole call chain. I suppose that's where you
would use a "mutable" in C++.

I did that to make sure there was no other cases like the one fixed in
the previous patch. I don't think it can hurt.

gdb/ChangeLog:

	* ada-varobj.c (ada_number_of_children): Constify struct varobj *
	parameter.
	(ada_name_of_variable): Same.
	(ada_path_expr_of_child): Same.
	(ada_value_of_variable): Same.
	(ada_value_is_changeable_p): Same.
	(ada_value_has_mutated): Same.
	* c-varobj.c (varobj_is_anonymous_child): Same.
	(c_is_path_expr_parent): Same.
	(c_number_of_children): Same.
	(c_name_of_variable): Same.
	(c_path_expr_of_child): Same.
	(get_type): Same.
	(c_value_of_variable): Same.
	(cplus_number_of_children): Same.
	(cplus_name_of_variable): Same.
	(cplus_path_expr_of_child): Same.
	(cplus_value_of_variable): Same.
	* jv-varobj.c (java_number_of_children): Same.
	(java_name_of_variable): Same.
	(java_path_expr_of_child): Same.
	(java_value_of_variable): Same.
	* varobj.c (number_of_children): Same.
	(name_of_variable): Same.
	(is_root_p): Same.
	(varobj_ensure_python_env): Same.
	(varobj_get_objname): Same.
	(varobj_get_expression): Same.
	(varobj_get_display_format): Same.
	(varobj_get_display_hint): Same.
	(varobj_has_more): Same.
	(varobj_get_thread_id): Same.
	(varobj_get_frozen): Same.
	(dynamic_varobj_has_child_method): Same.
	(varobj_get_gdb_type): Same.
	(is_path_expr_parent): Same.
	(varobj_default_is_path_expr_parent): Same.
	(varobj_get_language): Same.
	(varobj_get_attributes): Same.
	(varobj_is_dynamic_p): Same.
	(varobj_get_child_range): Same.
	(varobj_value_has_mutated): Same.
	(varobj_get_value_type): Same.
	(number_of_children): Same.
	(name_of_variable): Same.
	(check_scope): Same.
	(varobj_editable_p): Same.
	(varobj_value_is_changeable_p): Same.
	(varobj_floating_p): Same.
	(varobj_default_value_is_changeable_p): Same.
	* varobj.h (struct lang_varobj_ops): Consitfy some struct varobj *
	parameters.
	(varobj_get_objname): Constify struct varobj * parameter.
	(varobj_get_expression): Same.
	(varobj_get_thread_id): Same.
	(varobj_get_frozen): Same.
	(varobj_get_child_range): Same.
	(varobj_get_display_hint): Same.
	(varobj_get_gdb_type): Same.
	(varobj_get_language): Same.
	(varobj_get_attributes): Same.
	(varobj_editable_p): Same.
	(varobj_floating_p): Same.
	(varobj_has_more): Same.
	(varobj_is_dynamic_p): Same.
	(varobj_ensure_python_env): Same.
	(varobj_default_value_is_changeable_p): Same.
	(varobj_value_is_changeable_p): Same.
	(varobj_get_value_type): Same.
	(varobj_is_anonymous_child): Same.
	(varobj_value_get_print_value): Same.
	(varobj_default_is_path_expr_parent): Same.
---
 gdb/ada-varobj.c | 12 ++++++------
 gdb/c-varobj.c   | 22 ++++++++++-----------
 gdb/jv-varobj.c  |  8 ++++----
 gdb/varobj.c     | 60 ++++++++++++++++++++++++++++----------------------------
 gdb/varobj.h     | 56 ++++++++++++++++++++++++++--------------------------
 5 files changed, 79 insertions(+), 79 deletions(-)

diff --git a/gdb/ada-varobj.c b/gdb/ada-varobj.c
index 690ee49..3ecac9d 100644
--- a/gdb/ada-varobj.c
+++ b/gdb/ada-varobj.c
@@ -906,13 +906,13 @@ ada_varobj_get_value_of_variable (struct value *value,
 /* Ada specific callbacks for VAROBJs.  */
 
 static int
-ada_number_of_children (struct varobj *var)
+ada_number_of_children (const struct varobj *var)
 {
   return ada_varobj_get_number_of_children (var->value, var->type);
 }
 
 static char *
-ada_name_of_variable (struct varobj *parent)
+ada_name_of_variable (const struct varobj *parent)
 {
   return c_varobj_ops.name_of_variable (parent);
 }
@@ -925,7 +925,7 @@ ada_name_of_child (struct varobj *parent, int index)
 }
 
 static char*
-ada_path_expr_of_child (struct varobj *child)
+ada_path_expr_of_child (const struct varobj *child)
 {
   struct varobj *parent = child->parent;
   const char *parent_path_expr = varobj_get_path_expr (parent);
@@ -952,7 +952,7 @@ ada_type_of_child (struct varobj *parent, int index)
 }
 
 static char *
-ada_value_of_variable (struct varobj *var, enum varobj_display_formats format)
+ada_value_of_variable (const struct varobj *var, enum varobj_display_formats format)
 {
   struct value_print_options opts;
 
@@ -964,7 +964,7 @@ ada_value_of_variable (struct varobj *var, enum varobj_display_formats format)
 /* Implement the "value_is_changeable_p" routine for Ada.  */
 
 static int
-ada_value_is_changeable_p (struct varobj *var)
+ada_value_is_changeable_p (const struct varobj *var)
 {
   struct type *type = var->value ? value_type (var->value) : var->type;
 
@@ -990,7 +990,7 @@ ada_value_is_changeable_p (struct varobj *var)
 /* Implement the "value_has_mutated" routine for Ada.  */
 
 static int
-ada_value_has_mutated (struct varobj *var, struct value *new_val,
+ada_value_has_mutated (const struct varobj *var, struct value *new_val,
 		       struct type *new_type)
 {
   int i;
diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
index bd0e5fb..c706eb0 100644
--- a/gdb/c-varobj.c
+++ b/gdb/c-varobj.c
@@ -35,7 +35,7 @@ static void cplus_class_num_children (struct type *type, int children[3]);
    thing to do is to compare the child's name with ANONYMOUS_*_NAME.  */
 
 int
-varobj_is_anonymous_child (struct varobj *child)
+varobj_is_anonymous_child (const struct varobj *child)
 {
   return (strcmp (child->name, ANONYMOUS_STRUCT_NAME) == 0
 	  || strcmp (child->name, ANONYMOUS_UNION_NAME) == 0);
@@ -130,7 +130,7 @@ adjust_value_for_child_access (struct value **value,
    a valid path expression?  */
 
 static int
-c_is_path_expr_parent (struct varobj *var)
+c_is_path_expr_parent (const struct varobj *var)
 {
   struct type *type;
 
@@ -179,7 +179,7 @@ c_is_path_expr_parent (struct varobj *var)
 /* C */
 
 static int
-c_number_of_children (struct varobj *var)
+c_number_of_children (const struct varobj *var)
 {
   struct type *type = varobj_get_value_type (var);
   int children = 0;
@@ -230,7 +230,7 @@ c_number_of_children (struct varobj *var)
 }
 
 static char *
-c_name_of_variable (struct varobj *parent)
+c_name_of_variable (const struct varobj *parent)
 {
   return xstrdup (parent->name);
 }
@@ -431,7 +431,7 @@ c_name_of_child (struct varobj *parent, int index)
 }
 
 static char *
-c_path_expr_of_child (struct varobj *child)
+c_path_expr_of_child (const struct varobj *child)
 {
   char *path_expr;
 
@@ -462,7 +462,7 @@ c_type_of_child (struct varobj *parent, int index)
    to return the real type of the variable.  */
 
 static struct type *
-get_type (struct varobj *var)
+get_type (const struct varobj *var)
 {
   struct type *type;
 
@@ -474,7 +474,7 @@ get_type (struct varobj *var)
 }
 
 static char *
-c_value_of_variable (struct varobj *var, enum varobj_display_formats format)
+c_value_of_variable (const struct varobj *var, enum varobj_display_formats format)
 {
   /* BOGUS: if val_print sees a struct/class, or a reference to one,
      it will print out its children instead of "{...}".  So we need to
@@ -558,7 +558,7 @@ enum vsections
 /* C++ */
 
 static int
-cplus_number_of_children (struct varobj *var)
+cplus_number_of_children (const struct varobj *var)
 {
   struct value *value = NULL;
   struct type *type;
@@ -671,7 +671,7 @@ cplus_class_num_children (struct type *type, int children[3])
 }
 
 static char *
-cplus_name_of_variable (struct varobj *parent)
+cplus_name_of_variable (const struct varobj *parent)
 {
   return c_name_of_variable (parent);
 }
@@ -906,7 +906,7 @@ cplus_name_of_child (struct varobj *parent, int index)
 }
 
 static char *
-cplus_path_expr_of_child (struct varobj *child)
+cplus_path_expr_of_child (const struct varobj *child)
 {
   char *path_expr;
 
@@ -934,7 +934,7 @@ cplus_type_of_child (struct varobj *parent, int index)
 }
 
 static char *
-cplus_value_of_variable (struct varobj *var, 
+cplus_value_of_variable (const struct varobj *var,
 			 enum varobj_display_formats format)
 {
 
diff --git a/gdb/jv-varobj.c b/gdb/jv-varobj.c
index 40c1b95..1ad13cd 100644
--- a/gdb/jv-varobj.c
+++ b/gdb/jv-varobj.c
@@ -21,13 +21,13 @@
 /* Java */
 
 static int
-java_number_of_children (struct varobj *var)
+java_number_of_children (const struct varobj *var)
 {
   return cplus_varobj_ops.number_of_children (var);
 }
 
 static char *
-java_name_of_variable (struct varobj *parent)
+java_name_of_variable (const struct varobj *parent)
 {
   char *p, *name;
 
@@ -66,7 +66,7 @@ java_name_of_child (struct varobj *parent, int index)
 }
 
 static char *
-java_path_expr_of_child (struct varobj *child)
+java_path_expr_of_child (const struct varobj *child)
 {
   return NULL;
 }
@@ -84,7 +84,7 @@ java_type_of_child (struct varobj *parent, int index)
 }
 
 static char *
-java_value_of_variable (struct varobj *var, enum varobj_display_formats format)
+java_value_of_variable (const struct varobj *var, enum varobj_display_formats format)
 {
   return cplus_varobj_ops.value_of_variable (var, format);
 }
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 28d388e..0daef91 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -193,9 +193,9 @@ static int install_new_value (struct varobj *var, struct value *value,
 
 /* Language-specific routines.  */
 
-static int number_of_children (struct varobj *);
+static int number_of_children (const struct varobj *);
 
-static char *name_of_variable (struct varobj *);
+static char *name_of_variable (const struct varobj *);
 
 static char *name_of_child (struct varobj *, int);
 
@@ -206,7 +206,7 @@ static struct value *value_of_child (struct varobj *parent, int index);
 static char *my_value_of_variable (struct varobj *var,
 				   enum varobj_display_formats format);
 
-static int is_root_p (struct varobj *var);
+static int is_root_p (const struct varobj *var);
 
 static struct varobj *varobj_add_child (struct varobj *var,
 					struct varobj_item *item);
@@ -230,7 +230,7 @@ static struct vlist **varobj_table;
 
 /* API Implementation */
 static int
-is_root_p (struct varobj *var)
+is_root_p (const struct varobj *var)
 {
   return (var->root->rootvar == var);
 }
@@ -239,7 +239,7 @@ is_root_p (struct varobj *var)
 /* Helper function to install a Python environment suitable for
    use during operations on VAR.  */
 struct cleanup *
-varobj_ensure_python_env (struct varobj *var)
+varobj_ensure_python_env (const struct varobj *var)
 {
   return ensure_python_env (var->root->exp->gdbarch,
 			    var->root->exp->language_defn);
@@ -487,7 +487,7 @@ varobj_get_handle (char *objname)
 /* Given the handle, return the name of the object.  */
 
 char *
-varobj_get_objname (struct varobj *var)
+varobj_get_objname (const struct varobj *var)
 {
   return var->obj_name;
 }
@@ -496,7 +496,7 @@ varobj_get_objname (struct varobj *var)
    result must be freed by the caller.  */
 
 char *
-varobj_get_expression (struct varobj *var)
+varobj_get_expression (const struct varobj *var)
 {
   return name_of_variable (var);
 }
@@ -600,13 +600,13 @@ varobj_set_display_format (struct varobj *var,
 }
 
 enum varobj_display_formats
-varobj_get_display_format (struct varobj *var)
+varobj_get_display_format (const struct varobj *var)
 {
   return var->format;
 }
 
 char *
-varobj_get_display_hint (struct varobj *var)
+varobj_get_display_hint (const struct varobj *var)
 {
   char *result = NULL;
 
@@ -630,7 +630,7 @@ varobj_get_display_hint (struct varobj *var)
 /* Return true if the varobj has items after TO, false otherwise.  */
 
 int
-varobj_has_more (struct varobj *var, int to)
+varobj_has_more (const struct varobj *var, int to)
 {
   if (VEC_length (varobj_p, var->children) > to)
     return 1;
@@ -643,7 +643,7 @@ varobj_has_more (struct varobj *var, int to)
    inside that thread, returns GDB id of the thread -- which
    is always positive.  Otherwise, returns -1.  */
 int
-varobj_get_thread_id (struct varobj *var)
+varobj_get_thread_id (const struct varobj *var)
 {
   if (var->root->valid_block && var->root->thread_id > 0)
     return var->root->thread_id;
@@ -665,7 +665,7 @@ varobj_set_frozen (struct varobj *var, int frozen)
 }
 
 int
-varobj_get_frozen (struct varobj *var)
+varobj_get_frozen (const struct varobj *var)
 {
   return var->frozen;
 }
@@ -741,7 +741,7 @@ install_dynamic_child (struct varobj *var,
 #if HAVE_PYTHON
 
 static int
-dynamic_varobj_has_child_method (struct varobj *var)
+dynamic_varobj_has_child_method (const struct varobj *var)
 {
   struct cleanup *back_to;
   PyObject *printer = var->dynamic->pretty_printer;
@@ -991,7 +991,7 @@ varobj_get_type (struct varobj *var)
 /* Obtain the type of an object variable.  */
 
 struct type *
-varobj_get_gdb_type (struct varobj *var)
+varobj_get_gdb_type (const struct varobj *var)
 {
   return var->type;
 }
@@ -1000,7 +1000,7 @@ varobj_get_gdb_type (struct varobj *var)
    a valid path expression?  */
 
 static int
-is_path_expr_parent (struct varobj *var)
+is_path_expr_parent (const struct varobj *var)
 {
   gdb_assert (var->root->lang_ops->is_path_expr_parent != NULL);
   return var->root->lang_ops->is_path_expr_parent (var);
@@ -1011,7 +1011,7 @@ is_path_expr_parent (struct varobj *var)
    parent.  */
 
 int
-varobj_default_is_path_expr_parent (struct varobj *var)
+varobj_default_is_path_expr_parent (const struct varobj *var)
 {
   return 1;
 }
@@ -1048,13 +1048,13 @@ varobj_get_path_expr (struct varobj *var)
 }
 
 const struct language_defn *
-varobj_get_language (struct varobj *var)
+varobj_get_language (const struct varobj *var)
 {
   return var->root->exp->language_defn;
 }
 
 int
-varobj_get_attributes (struct varobj *var)
+varobj_get_attributes (const struct varobj *var)
 {
   int attributes = 0;
 
@@ -1068,7 +1068,7 @@ varobj_get_attributes (struct varobj *var)
 /* Return true if VAR is a dynamic varobj.  */
 
 int
-varobj_is_dynamic_p (struct varobj *var)
+varobj_is_dynamic_p (const struct varobj *var)
 {
   return var->dynamic->pretty_printer != NULL;
 }
@@ -1517,7 +1517,7 @@ install_new_value (struct varobj *var, struct value *value, int initial)
    selected sub-range of VAR.  If no range was selected using
    -var-set-update-range, then both will be -1.  */
 void
-varobj_get_child_range (struct varobj *var, int *from, int *to)
+varobj_get_child_range (const struct varobj *var, int *from, int *to)
 {
   *from = var->from;
   *to = var->to;
@@ -1579,7 +1579,7 @@ varobj_set_visualizer (struct varobj *var, const char *visualizer)
    NEW_VALUE may be NULL, if the varobj is now out of scope.  */
 
 static int
-varobj_value_has_mutated (struct varobj *var, struct value *new_value,
+varobj_value_has_mutated (const struct varobj *var, struct value *new_value,
 			  struct type *new_type)
 {
   /* If we haven't previously computed the number of children in var,
@@ -2209,7 +2209,7 @@ make_cleanup_free_variable (struct varobj *var)
 
    For example, top-level references are always stripped.  */
 struct type *
-varobj_get_value_type (struct varobj *var)
+varobj_get_value_type (const struct varobj *var)
 {
   struct type *type;
 
@@ -2278,7 +2278,7 @@ cppop (struct cpstack **pstack)
    is the number of children that the user will see in the variable
    display.  */
 static int
-number_of_children (struct varobj *var)
+number_of_children (const struct varobj *var)
 {
   return (*var->root->lang_ops->number_of_children) (var);
 }
@@ -2286,7 +2286,7 @@ number_of_children (struct varobj *var)
 /* What is the expression for the root varobj VAR? Returns a malloc'd
    string.  */
 static char *
-name_of_variable (struct varobj *var)
+name_of_variable (const struct varobj *var)
 {
   return (*var->root->lang_ops->name_of_variable) (var);
 }
@@ -2303,7 +2303,7 @@ name_of_child (struct varobj *var, int index)
    to it and return 1.  Otherwise, return 0.  */
 
 static int
-check_scope (struct varobj *var)
+check_scope (const struct varobj *var)
 {
   struct frame_info *fi;
   int scope;
@@ -2514,7 +2514,7 @@ varobj_formatted_print_options (struct value_print_options *opts,
 char *
 varobj_value_get_print_value (struct value *value,
 			      enum varobj_display_formats format,
-			      struct varobj *var)
+			      const struct varobj *var)
 {
   struct ui_file *stb;
   struct cleanup *old_chain;
@@ -2645,7 +2645,7 @@ varobj_value_get_print_value (struct value *value,
 }
 
 int
-varobj_editable_p (struct varobj *var)
+varobj_editable_p (const struct varobj *var)
 {
   struct type *type;
 
@@ -2673,7 +2673,7 @@ varobj_editable_p (struct varobj *var)
 /* Call VAR's value_is_changeable_p language-specific callback.  */
 
 int
-varobj_value_is_changeable_p (struct varobj *var)
+varobj_value_is_changeable_p (const struct varobj *var)
 {
   return var->root->lang_ops->value_is_changeable_p (var);
 }
@@ -2682,7 +2682,7 @@ varobj_value_is_changeable_p (struct varobj *var)
    selected frame, and not bound to thread/frame.  Such variable objects
    are created using '@' as frame specifier to -var-create.  */
 int
-varobj_floating_p (struct varobj *var)
+varobj_floating_p (const struct varobj *var)
 {
   return var->root->floating;
 }
@@ -2691,7 +2691,7 @@ varobj_floating_p (struct varobj *var)
    languages.  */
 
 int
-varobj_default_value_is_changeable_p (struct varobj *var)
+varobj_default_value_is_changeable_p (const struct varobj *var)
 {
   int r;
   struct type *type;
diff --git a/gdb/varobj.h b/gdb/varobj.h
index 145b963..8678889 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -167,11 +167,11 @@ struct varobj
 struct lang_varobj_ops
 {
   /* The number of children of PARENT.  */
-  int (*number_of_children) (struct varobj *parent);
+  int (*number_of_children) (const struct varobj *parent);
 
   /* The name (expression) of a root varobj.  The returned value must be freed
      by the caller.  */
-  char *(*name_of_variable) (struct varobj *parent);
+  char *(*name_of_variable) (const struct varobj *parent);
 
   /* The name of the INDEX'th child of PARENT.  The returned value must be
      freed by the caller.  */
@@ -180,7 +180,7 @@ struct lang_varobj_ops
   /* Returns the rooted expression of CHILD, which is a variable
      obtain that has some parent.  The returned value must be freed by the
      caller.  */
-  char *(*path_expr_of_child) (struct varobj *child);
+  char *(*path_expr_of_child) (const struct varobj *child);
 
   /* The ``struct value *'' of the INDEX'th child of PARENT.  The returned
      value must be freed by the caller.  */
@@ -191,7 +191,7 @@ struct lang_varobj_ops
 
   /* The current value of VAR.  The returned value must be freed by the
      caller.  */
-  char *(*value_of_variable) (struct varobj *var,
+  char *(*value_of_variable) (const struct varobj *var,
 			      enum varobj_display_formats format);
 
   /* Return non-zero if changes in value of VAR must be detected and
@@ -202,7 +202,7 @@ struct lang_varobj_ops
 
      Return value of 0 means that gdb need not call value_fetch_lazy
      for the value of this variable object.  */
-  int (*value_is_changeable_p) (struct varobj *var);
+  int (*value_is_changeable_p) (const struct varobj *var);
 
   /* Return nonzero if the type of VAR has mutated.
 
@@ -216,14 +216,14 @@ struct lang_varobj_ops
      children is set (not < 0).
 
      Languages where types do not mutate can set this to NULL.  */
-  int (*value_has_mutated) (struct varobj *var, struct value *new_value,
+  int (*value_has_mutated) (const struct varobj *var, struct value *new_value,
 			    struct type *new_type);
 
   /* Return nonzero if VAR is a suitable path expression parent.
 
      For C like languages with anonymous structures and unions an anonymous
      structure or union is not a suitable parent.  */
-  int (*is_path_expr_parent) (struct varobj *var);
+  int (*is_path_expr_parent) (const struct varobj *var);
 };
 
 extern const struct lang_varobj_ops c_varobj_ops;
@@ -242,9 +242,9 @@ extern char *varobj_gen_name (void);
 
 extern struct varobj *varobj_get_handle (char *name);
 
-extern char *varobj_get_objname (struct varobj *var);
+extern char *varobj_get_objname (const struct varobj *var);
 
-extern char *varobj_get_expression (struct varobj *var);
+extern char *varobj_get_expression (const struct varobj *var);
 
 extern int varobj_delete (struct varobj *var, char ***dellist,
 			  int only_children);
@@ -254,19 +254,19 @@ extern enum varobj_display_formats varobj_set_display_format (
 					enum varobj_display_formats format);
 
 extern enum varobj_display_formats varobj_get_display_format (
-							struct varobj *var);
+						const struct varobj *var);
 
-extern int varobj_get_thread_id (struct varobj *var);
+extern int varobj_get_thread_id (const struct varobj *var);
 
 extern void varobj_set_frozen (struct varobj *var, int frozen);
 
-extern int varobj_get_frozen (struct varobj *var);
+extern int varobj_get_frozen (const struct varobj *var);
 
-extern void varobj_get_child_range (struct varobj *var, int *from, int *to);
+extern void varobj_get_child_range (const struct varobj *var, int *from, int *to);
 
 extern void varobj_set_child_range (struct varobj *var, int from, int to);
 
-extern char *varobj_get_display_hint (struct varobj *var);
+extern char *varobj_get_display_hint (const struct varobj *var);
 
 extern int varobj_get_num_children (struct varobj *var);
 
@@ -283,13 +283,13 @@ extern VEC (varobj_p)* varobj_list_children (struct varobj *var,
 
 extern char *varobj_get_type (struct varobj *var);
 
-extern struct type *varobj_get_gdb_type (struct varobj *var);
+extern struct type *varobj_get_gdb_type (const struct varobj *var);
 
 extern char *varobj_get_path_expr (struct varobj *var);
 
-extern const struct language_defn *varobj_get_language (struct varobj *var);
+extern const struct language_defn *varobj_get_language (const struct varobj *var);
 
-extern int varobj_get_attributes (struct varobj *var);
+extern int varobj_get_attributes (const struct varobj *var);
 
 extern char *varobj_get_formatted_value (struct varobj *var,
 					 enum varobj_display_formats format);
@@ -306,33 +306,33 @@ extern VEC(varobj_update_result) *varobj_update (struct varobj **varp,
 
 extern void varobj_invalidate (void);
 
-extern int varobj_editable_p (struct varobj *var);
+extern int varobj_editable_p (const struct varobj *var);
 
-extern int varobj_floating_p (struct varobj *var);
+extern int varobj_floating_p (const struct varobj *var);
 
 extern void varobj_set_visualizer (struct varobj *var,
 				   const char *visualizer);
 
 extern void varobj_enable_pretty_printing (void);
 
-extern int varobj_has_more (struct varobj *var, int to);
+extern int varobj_has_more (const struct varobj *var, int to);
 
-extern int varobj_is_dynamic_p (struct varobj *var);
+extern int varobj_is_dynamic_p (const struct varobj *var);
 
-extern struct cleanup *varobj_ensure_python_env (struct varobj *var);
+extern struct cleanup *varobj_ensure_python_env (const struct varobj *var);
 
-extern int varobj_default_value_is_changeable_p (struct varobj *var);
-extern int varobj_value_is_changeable_p (struct varobj *var);
+extern int varobj_default_value_is_changeable_p (const struct varobj *var);
+extern int varobj_value_is_changeable_p (const struct varobj *var);
 
-extern struct type *varobj_get_value_type (struct varobj *var);
+extern struct type *varobj_get_value_type (const struct varobj *var);
 
-extern int varobj_is_anonymous_child (struct varobj *child);
+extern int varobj_is_anonymous_child (const struct varobj *child);
 
 extern struct varobj *varobj_get_path_expr_parent (struct varobj *var);
 
 extern char *varobj_value_get_print_value (struct value *value,
 					   enum varobj_display_formats format,
-					   struct varobj *var);
+					   const struct varobj *var);
 
 extern void varobj_formatted_print_options (struct value_print_options *opts,
 					    enum varobj_display_formats format);
@@ -340,6 +340,6 @@ extern void varobj_formatted_print_options (struct value_print_options *opts,
 extern void varobj_restrict_range (VEC (varobj_p) *children, int *from,
 				   int *to);
 
-extern int varobj_default_is_path_expr_parent (struct varobj *var);
+extern int varobj_default_is_path_expr_parent (const struct varobj *var);
 
 #endif /* VAROBJ_H */
-- 
2.1.4

Re: [PATCH 1/6] Free results of varobj_get_expression

[Joel Brobecker]

On Thu, Jan 29, 2015 at 02:28:31PM -0500, Simon Marchi wrote:
> varobj_get_expression returns an allocated string, which must be freed
> by the caller.
> 
> gdb/ChangeLog:
> 
> 	* mi-cmd-var.c (print_varobj): Free varobj_get_expression
> 	result.
> 	(mi_cmd_var_info_expression): Same.
> 	* varobj.c (varobj_get_expression): Mention in the comment that
> 	the result must by freed by the caller.

OK!

Thank you,
-- 
Joel

Re: [PATCH 2/6] Mention which return values need to be freed in lang_varobj_ops

[Joel Brobecker]

> gdb/ChangeLog:
> 
> 	* varobj.h (lang_varobj_ops): Mention which return values need
> 	to be freed.

Thanks for doing that! One question...

> -  /* The ``struct value *'' of the INDEX'th child of PARENT.  */
> +  /* The ``struct value *'' of the INDEX'th child of PARENT.  The returned
> +     value must be freed by the caller.  */
>    struct value *(*value_of_child) (struct varobj *parent, int index);

I'm really surprised by this. For memory management, the struct value
objects are put on a chain. So, you wouldn't delete the value returned,
but you would instead use "value_mark/value_free_to_mark". The top-level
command loop takes a mark at the beginning of the command, and uses it
to free any un-freed value after the command completes.

But maybe you saw something that contradicts my understanding?

-- 
Joel

Re: [PATCH 4/6] Constify some parameters in the varobj code

[Joel Brobecker]

> I did that to make sure there was no other cases like the one fixed in
> the previous patch. I don't think it can hurt.

Agreed.

> gdb/ChangeLog:
> 
> 	* ada-varobj.c (ada_number_of_children): Constify struct varobj *
> 	parameter.
> 	(ada_name_of_variable): Same.
> 	(ada_path_expr_of_child): Same.
> 	(ada_value_of_variable): Same.
> 	(ada_value_is_changeable_p): Same.
> 	(ada_value_has_mutated): Same.
> 	* c-varobj.c (varobj_is_anonymous_child): Same.
> 	(c_is_path_expr_parent): Same.
> 	(c_number_of_children): Same.
> 	(c_name_of_variable): Same.
> 	(c_path_expr_of_child): Same.
> 	(get_type): Same.
> 	(c_value_of_variable): Same.
> 	(cplus_number_of_children): Same.
> 	(cplus_name_of_variable): Same.
> 	(cplus_path_expr_of_child): Same.
> 	(cplus_value_of_variable): Same.
> 	* jv-varobj.c (java_number_of_children): Same.
> 	(java_name_of_variable): Same.
> 	(java_path_expr_of_child): Same.
> 	(java_value_of_variable): Same.
> 	* varobj.c (number_of_children): Same.
> 	(name_of_variable): Same.
> 	(is_root_p): Same.
> 	(varobj_ensure_python_env): Same.
> 	(varobj_get_objname): Same.
> 	(varobj_get_expression): Same.
> 	(varobj_get_display_format): Same.
> 	(varobj_get_display_hint): Same.
> 	(varobj_has_more): Same.
> 	(varobj_get_thread_id): Same.
> 	(varobj_get_frozen): Same.
> 	(dynamic_varobj_has_child_method): Same.
> 	(varobj_get_gdb_type): Same.
> 	(is_path_expr_parent): Same.
> 	(varobj_default_is_path_expr_parent): Same.
> 	(varobj_get_language): Same.
> 	(varobj_get_attributes): Same.
> 	(varobj_is_dynamic_p): Same.
> 	(varobj_get_child_range): Same.
> 	(varobj_value_has_mutated): Same.
> 	(varobj_get_value_type): Same.
> 	(number_of_children): Same.
> 	(name_of_variable): Same.
> 	(check_scope): Same.
> 	(varobj_editable_p): Same.
> 	(varobj_value_is_changeable_p): Same.
> 	(varobj_floating_p): Same.
> 	(varobj_default_value_is_changeable_p): Same.
> 	* varobj.h (struct lang_varobj_ops): Consitfy some struct varobj *
> 	parameters.
> 	(varobj_get_objname): Constify struct varobj * parameter.
> 	(varobj_get_expression): Same.
> 	(varobj_get_thread_id): Same.
> 	(varobj_get_frozen): Same.
> 	(varobj_get_child_range): Same.
> 	(varobj_get_display_hint): Same.
> 	(varobj_get_gdb_type): Same.
> 	(varobj_get_language): Same.
> 	(varobj_get_attributes): Same.
> 	(varobj_editable_p): Same.
> 	(varobj_floating_p): Same.
> 	(varobj_has_more): Same.
> 	(varobj_is_dynamic_p): Same.
> 	(varobj_ensure_python_env): Same.
> 	(varobj_default_value_is_changeable_p): Same.
> 	(varobj_value_is_changeable_p): Same.
> 	(varobj_get_value_type): Same.
> 	(varobj_is_anonymous_child): Same.
> 	(varobj_value_get_print_value): Same.
> 	(varobj_default_is_path_expr_parent): Same.

Sorry you had to write such as long ChangeLog entry. CL can be useful
at times, but really a timewaster at others :-(.

The patch looks pretty good to me, with a few corrections to avoid
some lines that became too long. Pre-approved with those adjustments.

-- 
Joel

Re: [PATCH 5/6] Mention that create_child takes ownership of the allocated name

[Joel Brobecker]

> gdb/ChangeLog:
> 
> 	* varobj.c (create_child): Modify comment.

OK, with one favor if you wouldn't mind.

> ---
>  gdb/varobj.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 0daef91..d3fa1ba 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -2043,7 +2043,9 @@ uninstall_variable (struct varobj *var)
>  
>  }
>  
> -/* Create and install a child of the parent of the given name.  */
> +/* Create and install a child of the parent of the given name.
> +
> +   The created VAROBJ takes ownership of the allocated NAME.  */
>  static struct varobj *

GDB's Coding Style requires that we have an empty line between
function documentation and function definition. It was missing
here, and since you're touching this area, would you mind adding it?

Thank you!

-- 
Joel

Re: [PATCH 6/6] Fix varobj_delete comment

[Joel Brobecker]

> gdb/ChangeLog:
> 
> 	* varobj.c (varobj_delete): Fix comment.

Looks good. Thank you!

-- 
Joel

Re: [PATCH 3/6] Set varobj->path_expr in varobj_get_path_expr

[Joel Brobecker]

> It seems like different languages are doing this differently (e.g.
> C and Ada). For C, var->path_expr is set inside c_path_expr_of_child.
> The next time the value is requested, is it therefore not recomputed.
> Ada does not set this field, but just returns the value. Since the field
> is never set, the value is recomputed every time it is requested.
> 
> This patch makes it so that path_expr_of_child's only job is to compute
> the path expression, not save/cache the value. The field is set by the
> varobj common code.

Nice little cleanup, IMO. Thanks for doing it.

> gdb/ChangeLog:
> 
> 	* varobj.c (varobj_get_path_expr): Set var->path_expr.
> 	* c-varobj.c (c_path_expr_of_child): Set local var instead of
> 	child->path_expr.
> 	(cplus_path_expr_of_child): Same.

LGTM. Thank you!

-- 
Joel

Re: [PATCH 2/6] Mention which return values need to be freed in lang_varobj_ops

[Simon Marchi]

On 15-01-29 10:28 PM, Joel Brobecker wrote:
>> gdb/ChangeLog:
>>
>> 	* varobj.h (lang_varobj_ops): Mention which return values need
>> 	to be freed.
> 
> Thanks for doing that! One question...
> 
>> -  /* The ``struct value *'' of the INDEX'th child of PARENT.  */
>> +  /* The ``struct value *'' of the INDEX'th child of PARENT.  The returned
>> +     value must be freed by the caller.  */
>>    struct value *(*value_of_child) (struct varobj *parent, int index);
> 
> I'm really surprised by this. For memory management, the struct value
> objects are put on a chain. So, you wouldn't delete the value returned,
> but you would instead use "value_mark/value_free_to_mark". The top-level
> command loop takes a mark at the beginning of the command, and uses it
> to free any un-freed value after the command completes.
> 
> But maybe you saw something that contradicts my understanding?

After looking more closely, I think you are right. Originally, I saw that
install_new_value called value_free on the old value and jumped to the
conclusion. Actually, value_free is more like a "value_decref", which
frees the variable if the reference count drops to 0. The call to
value_free just matches the value_incref that was also done in
install_new_value when we installed the value. So just calling
value_of_child doesn't mean that you have to call value_free.

Thanks for the explanation, I didn't know about the memory management of
values. I'll remove the comment change for value_of_child. Is the rest of
the patch ok?

Re: [PATCH 1/6] Free results of varobj_get_expression

[Simon Marchi]

On 15-01-29 10:12 PM, Joel Brobecker wrote:
> On Thu, Jan 29, 2015 at 02:28:31PM -0500, Simon Marchi wrote:
>> varobj_get_expression returns an allocated string, which must be freed
>> by the caller.
>>
>> gdb/ChangeLog:
>>
>> 	* mi-cmd-var.c (print_varobj): Free varobj_get_expression
>> 	result.
>> 	(mi_cmd_var_info_expression): Same.
>> 	* varobj.c (varobj_get_expression): Mention in the comment that
>> 	the result must by freed by the caller.
> 
> OK!
> 
> Thank you,

Pushed, thanks!

Re: [PATCH 3/6] Set varobj->path_expr in varobj_get_path_expr

[Simon Marchi]

On 15-01-29 10:39 PM, Joel Brobecker wrote:
>> It seems like different languages are doing this differently (e.g.
>> C and Ada). For C, var->path_expr is set inside c_path_expr_of_child.
>> The next time the value is requested, is it therefore not recomputed.
>> Ada does not set this field, but just returns the value. Since the field
>> is never set, the value is recomputed every time it is requested.
>>
>> This patch makes it so that path_expr_of_child's only job is to compute
>> the path expression, not save/cache the value. The field is set by the
>> varobj common code.
> 
> Nice little cleanup, IMO. Thanks for doing it.
> 
>> gdb/ChangeLog:
>>
>> 	* varobj.c (varobj_get_path_expr): Set var->path_expr.
>> 	* c-varobj.c (c_path_expr_of_child): Set local var instead of
>> 	child->path_expr.
>> 	(cplus_path_expr_of_child): Same.
> 
> LGTM. Thank you!

Pushed, thanks!

Re: [PATCH 4/6] Constify some parameters in the varobj code

[Simon Marchi]

On 15-01-29 10:33 PM, Joel Brobecker wrote:
>> 	...
> 
> Sorry you had to write such as long ChangeLog entry. CL can be useful
> at times, but really a timewaster at others :-(.

I tried to use mklog, but it wouldn't recognize the function names :(.

> The patch looks pretty good to me, with a few corrections to avoid
> some lines that became too long. Pre-approved with those adjustments.

Pushed, with those lines wrapped (about 5 of them).

Re: [PATCH 5/6] Mention that create_child takes ownership of the allocated name

[Simon Marchi]

On 15-01-29 10:35 PM, Joel Brobecker wrote:
>> gdb/ChangeLog:
>>
>> 	* varobj.c (create_child): Modify comment.
> 
> OK, with one favor if you wouldn't mind.
> 
>> ---
>>  gdb/varobj.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/varobj.c b/gdb/varobj.c
>> index 0daef91..d3fa1ba 100644
>> --- a/gdb/varobj.c
>> +++ b/gdb/varobj.c
>> @@ -2043,7 +2043,9 @@ uninstall_variable (struct varobj *var)
>>  
>>  }
>>  
>> -/* Create and install a child of the parent of the given name.  */
>> +/* Create and install a child of the parent of the given name.
>> +
>> +   The created VAROBJ takes ownership of the allocated NAME.  */
>>  static struct varobj *
> 
> GDB's Coding Style requires that we have an empty line between
> function documentation and function definition. It was missing
> here, and since you're touching this area, would you mind adding it?
> 
> Thank you!

Thanks, pushed with an extra empty line.

Re: [PATCH 6/6] Fix varobj_delete comment

[Simon Marchi]

On 15-01-29 10:36 PM, Joel Brobecker wrote:
>> gdb/ChangeLog:
>>
>> 	* varobj.c (varobj_delete): Fix comment.
> 
> Looks good. Thank you!

Thanks, pushed!

Re: [PATCH 2/6] Mention which return values need to be freed in lang_varobj_ops

[Joel Brobecker]

> After looking more closely, I think you are right. Originally, I saw that
> install_new_value called value_free on the old value and jumped to the
> conclusion. Actually, value_free is more like a "value_decref", which
> frees the variable if the reference count drops to 0. The call to
> value_free just matches the value_incref that was also done in
> install_new_value when we installed the value. So just calling
> value_of_child doesn't mean that you have to call value_free.

Good :).

> Thanks for the explanation, I didn't know about the memory management of
> values. I'll remove the comment change for value_of_child. Is the rest of
> the patch ok?

Yes, it is. Go ahead and push the version without this particular
comment change.

-- 
Joel

Re: [PATCH 2/6] Mention which return values need to be freed in lang_varobj_ops

[Simon Marchi]

On 15-01-30 10:20 PM, Joel Brobecker wrote:
>> After looking more closely, I think you are right. Originally, I saw that
>> install_new_value called value_free on the old value and jumped to the
>> conclusion. Actually, value_free is more like a "value_decref", which
>> frees the variable if the reference count drops to 0. The call to
>> value_free just matches the value_incref that was also done in
>> install_new_value when we installed the value. So just calling
>> value_of_child doesn't mean that you have to call value_free.
> 
> Good :).
> 
>> Thanks for the explanation, I didn't know about the memory management of
>> values. I'll remove the comment change for value_of_child. Is the rest of
>> the patch ok?
> 
> Yes, it is. Go ahead and push the version without this particular
> comment change.
> 

Thanks, pushed.

Re: [PATCH 4/6] Constify some parameters in the varobj code

[Pedro Alves]

On 01/29/2015 08:28 PM, Simon Marchi wrote:
> To make it clear that some functions should not modify the variable
> object, this patch adds the const qualifier where it makes sense to some
> struct varobj * parameters. Most getters should take a const pointer to
> guarantee they don't modify the object.
> 
> Unfortunately, I couldn't add it to some callbacks (such as name_of_child).
> In the C implementation, they call c_describe_child, which calls
> varobj_get_path_expr. varobj_get_path_expr needs to modify the object in
> order to cache the computed value. It therefore can't take a const
> pointer, and it affects the whole call chain. I suppose that's where you
> would use a "mutable" in C++.

FYI, in these cases it's totally fine to have the function take a const pointer,
and then cast away the const inside the function.  We do exactly that in
a few places.  E.g.:

/* Returns the decoded name of GSYMBOL, as for ada_decode, caching it
   in the language-specific part of GSYMBOL, if it has not been
   previously computed.  Tries to save the decoded name in the same
   obstack as GSYMBOL, if possible, and otherwise on the heap (so that,
   in any case, the decoded symbol has a lifetime at least that of
   GSYMBOL).
   The GSYMBOL parameter is "mutable" in the C++ sense: logically
   const, but nevertheless modified to a semantically equivalent form
   when a decoded name is cached in it.  */

const char *
ada_decode_symbol (const struct general_symbol_info *arg)
{
  struct general_symbol_info *gsymbol = (struct general_symbol_info *) arg;
...

Thanks,
Pedro Alves

Re: [PATCH 4/6] Constify some parameters in the varobj code

[Simon Marchi]

On 15-02-03 11:55 AM, Pedro Alves wrote:
> On 01/29/2015 08:28 PM, Simon Marchi wrote:
>> To make it clear that some functions should not modify the variable
>> object, this patch adds the const qualifier where it makes sense to some
>> struct varobj * parameters. Most getters should take a const pointer to
>> guarantee they don't modify the object.
>>
>> Unfortunately, I couldn't add it to some callbacks (such as name_of_child).
>> In the C implementation, they call c_describe_child, which calls
>> varobj_get_path_expr. varobj_get_path_expr needs to modify the object in
>> order to cache the computed value. It therefore can't take a const
>> pointer, and it affects the whole call chain. I suppose that's where you
>> would use a "mutable" in C++.
> 
> FYI, in these cases it's totally fine to have the function take a const pointer,
> and then cast away the const inside the function.  We do exactly that in
> a few places.  E.g.:
> 
> /* Returns the decoded name of GSYMBOL, as for ada_decode, caching it
>    in the language-specific part of GSYMBOL, if it has not been
>    previously computed.  Tries to save the decoded name in the same
>    obstack as GSYMBOL, if possible, and otherwise on the heap (so that,
>    in any case, the decoded symbol has a lifetime at least that of
>    GSYMBOL).
>    The GSYMBOL parameter is "mutable" in the C++ sense: logically
>    const, but nevertheless modified to a semantically equivalent form
>    when a decoded name is cached in it.  */
> 
> const char *
> ada_decode_symbol (const struct general_symbol_info *arg)
> {
>   struct general_symbol_info *gsymbol = (struct general_symbol_info *) arg;
> ...
> 
> Thanks,
> Pedro Alves

Thanks for the tip! I'll do it soon.