Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts <nickrob@snap.net.nz>]

 > I think the checking of lvalue-ness is a very good change. I have some
 > comments, however:
 > 
 > 1. In varobj_editable_p you call gdb_evaluate_expression, and I believe this
 > to be wrong. We call gdb_evaluate_expression when we create varobj, and it
 > either succeeds, eventually setting varobj->value to something, or it does
 > not. There's no point to call gdb_evaluate_expression again. 

If gdb_evaluate_expression fails in varobj_create, a variable object is still
created, but just with an undefined value.  It needs to be called to get
value for VALUE_LVAL.

 >                                                               Further,
 > in varobj_create, gdb_evaluate_expression is called in specific frame,
 > and varobj_editable_p calls it in current frame. 

The current frame should be the frame in which the variable object is defined.
Can you explain why that's a problem?

 >                                                  Also, if
 > gdb_evaluate_expression fails, you xfree(exp). Where is 'exp' assigned a
 > value?

It's not needed.  I'll remove it from the patch.

 > 2. In varobj_value_is_changeable_p, you have changed from returning 'r' at
 > the end of function, to returning in several places. I don't think this
 > change has any effect on logic and therefore, if committed, should be
 > committed separately.  And, I actually prefer the original code -- return in
 > one place makes logic simpler.

It used to be a bigger change.  It's a consistent style with other functions
in varobj.c but maybe it's gratuitous.  I don't mind leaving it out.

 > 3. I think your change to c_name_of_variable should be a separate patch. I
 > also not sure it's right. Consider java_name_of_variable -- it calls 
 > cplus_name_of_variable and then does some quoting. With your change 
 > cplus_name_of_variable will return varobj->name, the the following code will
 > directly modify it. Is it intended?

Perhaps the right place for savestring, or better xsprintf, is in
java_name_of_variable.  Alternatively when varobj_get_expression is called
in mi-cmd-var.c, it's value should be freed.  I'll remove this change for now.

 > 4. I don't think your test actually tests that the 'editable' attribute comes
 > out as 'false'.

I'm not sure what to say.  It shows that if you try to assign a value to a 
cast GDB says "Variable object is not editable".

The error message for a variable object of a cast used to be:

&"mi_cmd_var_assign: Could not assign expression to variable object\n"
^error,msg="mi_cmd_var_assign: Could not assign expression to variable object"
(gdb) 

I could add another test for -var-show-attributes which will now give:

^done,attr="noneditable"

for a cast but I never use this command.

-- 
Nick                                           http://www.inet.net.nz/~nickrob