This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] MI: lvalues and variable_editable
> 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