From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9876 invoked by alias); 27 Oct 2007 06:25:00 -0000 Received: (qmail 9867 invoked by uid 22791); 27 Oct 2007 06:24:59 -0000 X-Spam-Check-By: sourceware.org Received: from zigzag.lvk.cs.msu.su (HELO zigzag.lvk.cs.msu.su) (158.250.17.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 27 Oct 2007 06:24:56 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.50) id 1Ilf6F-0002cE-BZ for gdb-patches@sources.redhat.com; Sat, 27 Oct 2007 10:24:53 +0400 Received: from localhost ([127.0.0.1] helo=ip6-localhost) by zigzag.lvk.cs.msu.su with esmtp (Exim 4.50) id 1Ilf64-0002bi-0l; Sat, 27 Oct 2007 10:24:36 +0400 From: Vladimir Prus Subject: Re: [PATCH] MI: lvalues and variable_editable To: Nick Roberts , gdb-patches@sources.redhat.com Date: Sat, 27 Oct 2007 06:29:00 -0000 References: <18210.27153.559569.601092@kahikatea.snap.net.nz> User-Agent: KNode/0.10.4 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7Bit Message-Id: 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/msg00734.txt.bz2 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