From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8668 invoked by alias); 27 Oct 2007 12:05:57 -0000 Received: (qmail 8540 invoked by uid 22791); 27 Oct 2007 12:05:55 -0000 X-Spam-Check-By: sourceware.org Received: from viper.snap.net.nz (HELO viper.snap.net.nz) (202.37.101.8) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 27 Oct 2007 12:05:52 +0000 Received: from kahikatea.snap.net.nz (189.61.255.123.dynamic.snap.net.nz [123.255.61.189]) by viper.snap.net.nz (Postfix) with ESMTP id 766A13D9D7C; Sun, 28 Oct 2007 01:05:46 +1300 (NZDT) Received: by kahikatea.snap.net.nz (Postfix, from userid 1000) id D13A18FC6D; Sun, 28 Oct 2007 01:05:33 +1300 (NZDT) From: Nick Roberts MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <18211.10636.539778.664602@kahikatea.snap.net.nz> Date: Sat, 27 Oct 2007 12:15:00 -0000 To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH] MI: lvalues and variable_editable In-Reply-To: References: <18210.27153.559569.601092@kahikatea.snap.net.nz> X-Mailer: VM 7.19 under Emacs 23.0.50.35 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-10/txt/msg00741.txt.bz2 > 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