This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] MI: lvalues and variable_editable
- From: Vladimir Prus <ghost at cs dot msu dot su>
- To: Nick Roberts <nickrob at snap dot net dot nz>, gdb-patches at sources dot redhat dot com
- Date: Sat, 27 Oct 2007 10:24:35 +0400
- Subject: Re: [PATCH] MI: lvalues and variable_editable
- References: <18210.27153.559569.601092@kahikatea.snap.net.nz>
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