-var-info-path-expression

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

-var-info-path-expression

Vladimir Prus

This patch implements new MI command, -var-info-path-expression,
that, given a variable object, returns full expression that corresponds
to it.  Both KDevelop and Eclipse have now code that guesses such
full expression, and that code is rather hacky, and not exactly correct.

Moreover, as soon as MI is taught to get the true type
polymorphic C++ objects and display fields of the real type, such full expression
cannot be computed in frontend at all.

The essence of this patch -- which expressions are returned in which cases -- are
ported without change from the Apple branch. The code structure is much
different though -- this patch capitalizes on MI refactoring patches I've posted
recently.

This is lightly tested by hand, I'll write automated tests later. There's no docs
either -- again, will be written after discussion.

There is a couple of issues with this patch:

        - I don't much like 'var-info-path-expression' name, but
        naming of MI commands is not very important.
        - I'm not sure why we can't report full expression in the
        output of -var-list-children. The code I have does not seem
        very computationally expensive.

Comments are appreciated.

- Volodya

        Implement -var-info-path-expression.
        * mi/mi-cmds.h (mi_cmd_var_info_path_expression):
        Declare.
        * mi/mi-cmds.c (mi_cmds): Register var-info-path-expression.
        * mi/mi-cmd-var.c (mi_cmd_var_info_path_expression): New.
        * varobj.c (struct varobj): New field 'path_expr'.
        (c_path_expr_of_child, cplus_path_expr_of_child)
        (java_path_expr_of_child): New.
        (struct language_specific): New field path_expr_of_child.
        (varobj_create): Initialize the path_expr field.
        (varobj_get_path_expr): New.
        (new_variable): Initialize the path_expr field.
        (free_variable): Free the path_expr field.
        (adjust_value_for_children_access): New parameter
        WAS_TYPE.
        (c_number_of_children): Adjust.
        (c_describe_child): New parameter CFULL_EXPRESSION.
        Compute full expression.
        (c_value_of_child, c_type_of_child): Adjust.
        (cplus_number_of_children): Adjust.
        (cplus_describe_child): New parameter CFULL_EXPRESSION.
        Compute full expression.
        (cplus_name_of_child, cplus_value_of_child)
        (cplus_type_of_child): Adjust.
        * varobj.h (varobj_get_path_expr): Declare.

path_expression__gdb_mainline.diff (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: -var-info-path-expression

Daniel Jacobowitz-2
On Mon, Dec 25, 2006 at 12:00:42PM +0300, Vladimir Prus wrote:

>
> This patch implements new MI command, -var-info-path-expression,
> that, given a variable object, returns full expression that corresponds
> to it.  Both KDevelop and Eclipse have now code that guesses such
> full expression, and that code is rather hacky, and not exactly correct.
>
> Moreover, as soon as MI is taught to get the true type
> polymorphic C++ objects and display fields of the real type, such full expression
> cannot be computed in frontend at all.
>
> The essence of this patch -- which expressions are returned in which cases -- are
> ported without change from the Apple branch. The code structure is much
> different though -- this patch capitalizes on MI refactoring patches I've posted
> recently.
>
> This is lightly tested by hand, I'll write automated tests later. There's no docs
> either -- again, will be written after discussion.
>
> There is a couple of issues with this patch:
>
> - I don't much like 'var-info-path-expression' name, but
> naming of MI commands is not very important.

True - I think it's fine.

> - I'm not sure why we can't report full expression in the
> output of -var-list-children. The code I have does not seem
> very computationally expensive.

Also true.  If this would be more useful, I'd be happy to do it that
way - would you still need -var-info-path-expression?

Having digested the patch, it looks generally OK.  I didn't really
proofread it; I'll save that until there's some docs and tests, and
the earlier patches are in.  Sorry, too much at once for me.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: -var-info-path-expression

Vladimir Prus
On Thursday 04 January 2007 01:39, Daniel Jacobowitz wrote:

> On Mon, Dec 25, 2006 at 12:00:42PM +0300, Vladimir Prus wrote:
> >
> > This patch implements new MI command, -var-info-path-expression,
> > that, given a variable object, returns full expression that corresponds
> > to it.  Both KDevelop and Eclipse have now code that guesses such
> > full expression, and that code is rather hacky, and not exactly correct.
> >
> > Moreover, as soon as MI is taught to get the true type
> > polymorphic C++ objects and display fields of the real type, such full expression
> > cannot be computed in frontend at all.
> >
> > The essence of this patch -- which expressions are returned in which cases -- are
> > ported without change from the Apple branch. The code structure is much
> > different though -- this patch capitalizes on MI refactoring patches I've posted
> > recently.
> >
> > This is lightly tested by hand, I'll write automated tests later. There's no docs
> > either -- again, will be written after discussion.
> >
> > There is a couple of issues with this patch:
> >
> > - I don't much like 'var-info-path-expression' name, but
> > naming of MI commands is not very important.
>
> True - I think it's fine.
>
> > - I'm not sure why we can't report full expression in the
> > output of -var-list-children. The code I have does not seem
> > very computationally expensive.
>
> Also true.  If this would be more useful, I'd be happy to do it that
> way - would you still need -var-info-path-expression?

No, I think -var-info-path-expression will not be needed then. Except if we
do it that way, we'll have "all of test are broken because they don't expect
an extra field" situation. Grr. I'm not sure what to do here.

- Volodya

Reply | Threaded
Open this post in threaded view
|

Re: -var-info-path-expression

Vladimir Prus
On Friday 05 January 2007 12:14, Vladimir Prus wrote:

> > > This is lightly tested by hand, I'll write automated tests later. There's no docs
> > > either -- again, will be written after discussion.
> > >
> > > There is a couple of issues with this patch:
> > >
> > > - I don't much like 'var-info-path-expression' name, but
> > > naming of MI commands is not very important.
> >
> > True - I think it's fine.
> >
> > > - I'm not sure why we can't report full expression in the
> > > output of -var-list-children. The code I have does not seem
> > > very computationally expensive.
> >
> > Also true.  If this would be more useful, I'd be happy to do it that
> > way - would you still need -var-info-path-expression?
>
> No, I think -var-info-path-expression will not be needed then. Except if we
> do it that way, we'll have "all of test are broken because they don't expect
> an extra field" situation. Grr. I'm not sure what to do here.
I guess I do know -- I don't care about minor interface details. It's more
important to have this implemented than solving "attribute vs. command"
question the right way, and therefore, using a separate command is fine.

Do you want me to add docs/tests or you can review the current
version of the patch, reposted here fore convenience?

- Volodya

        Implement -var-info-path-expression.
        * mi/mi-cmds.h (mi_cmd_var_info_path_expression):
        Declare.
        * mi/mi-cmds.c (mi_cmds): Register var-info-path-expression.
        * mi/mi-cmd-var.c (mi_cmd_var_info_path_expression): New.
        * varobj.c (struct varobj): New field 'path_expr'.
        (c_path_expr_of_child, cplus_path_expr_of_child)
        (java_path_expr_of_child): New.
        (struct language_specific): New field path_expr_of_child.
        (varobj_create): Initialize the path_expr field.
        (varobj_get_path_expr): New.
        (new_variable): Initialize the path_expr field.
        (free_variable): Free the path_expr field.
        (adjust_value_for_children_access): New parameter
        WAS_TYPE.
        (c_number_of_children): Adjust.
        (c_describe_child): New parameter CFULL_EXPRESSION.
        Compute full expression.
        (c_value_of_child, c_type_of_child): Adjust.
        (cplus_number_of_children): Adjust.
        (cplus_describe_child): New parameter CFULL_EXPRESSION.
        Compute full expression.
        (cplus_name_of_child, cplus_value_of_child)
        (cplus_type_of_child): Adjust.
        * varobj.h (varobj_get_path_expr): Declare.


path_expression__gdb_mainline.diff (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: -var-info-path-expression

Nick Roberts
 >  c_describe_child (struct varobj *parent, int index,
 > -  char **cname, struct value **cvalue, struct type **ctype)
 > +  char **cname, struct value **cvalue, struct type **ctype,
 > +  char **cfull_expression)

This argument list gets longer but, apart from only parent and index, only
one argument is non-null at any one time.  Would it be better to have

enum varobj_child_properties
  {
    CHILD_NAME,
    CHILD_VALUE,
    CHILD_TYPE,
    CHILD_FULL_EXPRESSION
  }

static void *
c_describe_child (enum varobj_child_properties property,
                  struct varobj *parent, int index)

and propagate these changes back to struct language_specific so we have:

static char *
name_of_child (struct varobj *var, int index)
{
  return (char *) (*var->root->lang->describe_child) (CHILD_NAME, var, index);
}

etc?

--
Nick                                           http://www.inet.net.nz/~nickrob
Reply | Threaded
Open this post in threaded view
|

Re: -var-info-path-expression

Vladimir Prus
On Sunday 28 January 2007 03:46, Nick Roberts wrote:
>  >  c_describe_child (struct varobj *parent, int index,
>  > -  char **cname, struct value **cvalue, struct type **ctype)
>  > +  char **cname, struct value **cvalue, struct type **ctype,
>  > +  char **cfull_expression)
>
> This argument list gets longer but, apart from only parent and index, only
> one argument is non-null at any one time.  

A future patch is very likely to change this. In particular, the 'create_child' function
needs both name of the child and its value. I might kill separate function
pointers in struct language_specific and use just single describe_child and
make create_child pass two non-NULL pointers?

> Would it be better to have
>
> enum varobj_child_properties
>   {
>     CHILD_NAME,
>     CHILD_VALUE,
>     CHILD_TYPE,
>     CHILD_FULL_EXPRESSION
>   }
>
> static void *
> c_describe_child (enum varobj_child_properties property,
>                   struct varobj *parent, int index)
>
> and propagate these changes back to struct language_specific so we have:
>
> static char *
> name_of_child (struct varobj *var, int index)
> {
>   return (char *) (*var->root->lang->describe_child) (CHILD_NAME, var, index);

And have casts from void* to the right type? I'm not sure that's any advantage.

- Volodya
Reply | Threaded
Open this post in threaded view
|

Re: -var-info-path-expression

Nick Roberts
 > > and propagate these changes back to struct language_specific so we have:
 > >
 > > static char *
 > > name_of_child (struct varobj *var, int index)
 > > {
 > >   return (char *) (*var->root->lang->describe_child) (CHILD_NAME, var, index);
 >
 > And have casts from void* to the right type? I'm not sure that's any
 > advantage.

Maybe using making a cast is as sinful as using a goto statement, I wouldn't
know, but I would call having four times fewer functions an advantage.

Perhaps it would be better to use a macro e.g

#define name_of_child(var, index)   \
(char *) (*var->root->lang->describe_child) (CHILD_NAME, var, index)


--
Nick                                           http://www.inet.net.nz/~nickrob
Reply | Threaded
Open this post in threaded view
|

Re: -var-info-path-expression

Vladimir Prus
On Sunday 28 January 2007 11:34, Nick Roberts wrote:

>  > > and propagate these changes back to struct language_specific so we have:
>  > >
>  > > static char *
>  > > name_of_child (struct varobj *var, int index)
>  > > {
>  > >   return (char *) (*var->root->lang->describe_child) (CHILD_NAME, var, index);
>  >
>  > And have casts from void* to the right type? I'm not sure that's any
>  > advantage.
>
> Maybe using making a cast is as sinful as using a goto statement, I wouldn't
> know, but I would call having four times fewer functions an advantage.

"functions"? You mean having just one function pointer in language_specific? Yes,
I agree that would be superiour and I plan to make such a change, separately.

> Perhaps it would be better to use a macro e.g
>
> #define name_of_child(var, index)   \
> (char *) (*var->root->lang->describe_child) (CHILD_NAME, var, index)

I'm not sure, I don't quite see reason to introduce macros if we can
avoid them.

- Volodya



Reply | Threaded
Open this post in threaded view
|

Re: -var-info-path-expression

Daniel Jacobowitz-2
In reply to this post by Vladimir Prus
On Sun, Jan 28, 2007 at 12:48:36AM +0300, Vladimir Prus wrote:
> I guess I do know -- I don't care about minor interface details. It's more
> important to have this implemented than solving "attribute vs. command"
> question the right way, and therefore, using a separate command is fine.
>
> Do you want me to add docs/tests or you can review the current
> version of the patch, reposted here fore convenience?

I'll just review this copy.  I think having it pop out of
-var-list-children automatically would be useful, but the testsuite
updates would be a pain - maybe we should keep that in mind when
we add tests for new MI commands and try to use more functions.

> +  if (argc != 1)
> +    error ("mi_cmd_var_info_path_expression: Usage: NAME.");
> +
> +  /* Get varobj handle, if a valid var obj name was specified */
> +  var = varobj_get_handle (argv[0]);
> +  if (var == NULL)
> +    error ("mi_cmd_var_info_path_expression: Variable object not found");

Missing _().  Also, we didn't update existing commands, but I think we
decided the "function:" prefixes weren't helpful.

> @@ -757,6 +779,21 @@ varobj_get_gdb_type (struct varobj *var)
>    return var->type;
>  }
>  
> +/* Return a pointer to the full rooted expression of varobj VAR.
> +   If it has not been computed yet, compute it */
> +char *
> +varobj_get_path_expr (struct varobj *var)
> +{
> +  if (var->path_expr != NULL)
> +    return var->path_expr;
> +  else if (is_root_p (var))
> +    return var->name;
> +  else
> +    {
> +      return (*var->root->lang->path_expr_of_child) (var);
> +    }
> +}

Since you initialize path_expr at the same time as name, will is_root_p
ever trigger here?

> @@ -1826,10 +1875,13 @@ value_struct_element_index (struct value
>     to NULL.  */
>  static void
>  c_describe_child (struct varobj *parent, int index,
> -  char **cname, struct value **cvalue, struct type **ctype)
> +  char **cname, struct value **cvalue, struct type **ctype,
> +  char **cfull_expression)

Nick's got a point about the growing number of arguments.  Would
converting them to a struct simplify it?

struct varobj_child_desc
{
  char *name;
  struct value *value;
  struct type *type;
  char *full_expression;
};

Hey... those fields all live in struct varobj... I wonder if this code
ought to be rearranged so that this initializes the child's struct
varobj.  But anyway let's not do that right now.  The new argument is
fine, I was just thinking out loud.

--
Daniel Jacobowitz
CodeSourcery