Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus <ghost@cs.msu.su>]

Nick Roberts wrote:

> 
> I'm representing a patch from before the release of GDB 6.7.  The change
> to c_name_of_variable is unrelated and I can commit it separately, if
> wished.
> 
> I also attach a test for the main change.

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. Further,
in varobj_create, gdb_evaluate_expression is called in specific frame,
and varobj_editable_p calls it in current frame. Also, if gdb_evaluate_expression
fails, you xfree(exp). Where is 'exp' assigned a value? 

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.

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?

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

Thanks,
Volodya