From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12325 invoked by alias); 8 Oct 2013 21:03:22 -0000 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 Received: (qmail 12309 invoked by uid 89); 8 Oct 2013 21:03:22 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qe0-f74.google.com Received: from mail-qe0-f74.google.com (HELO mail-qe0-f74.google.com) (209.85.128.74) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 08 Oct 2013 21:03:20 +0000 Received: by mail-qe0-f74.google.com with SMTP id a11so903708qen.5 for ; Tue, 08 Oct 2013 14:03:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=CRqWUyI65A1LD4CXLo5nziP5ReaPE0i5HSL9aWw/WGY=; b=dPHbQ3NKc7cKY/B7nIckMYXcwX6HzWUA7IeyvRCZqeycgqXhEDOYWEhc7kVYFSeVSB PeRpoFn2gZHJ1WFMN7xfKDlGe9hrUvu7vVecU90yTtE0h0hw3dMQE1wnyZJvQFSIjwXG 0W5rRtEy3g5R3mtxAYG5laH6xsVqHugVGx7Nxi/0xUZaFMDkbuJ9Hu6vZ4jGogpZkqjp oQhc23yXeTZdvEnLpW4hj/VBO0gaCiw/mNeXVqPK7lmaWhyK4SF8Im2akTldXROLNJ9W WsKZLfekFSqJqpeLZ6EY5jju9xPctpttpj2pnbxaCF+is6gsG1D59TF3c3o3kmpwig2E DZBQ== X-Gm-Message-State: ALoCoQmN27mdegwUBB4ff9yHx0OUAMIuCCTKvmrlIuoPj65g6O/2P++iO6QgtUg7/uXua+Y6QuDKzhr0TyxJsvhSYq+sRAELlSd86MxI9VNsux/HHJzd6tAl9Ujtlj6wCtVGzSAKSjMxGqB1dvc96Ud/YjyDWqQ+BuYDyLrCsMbr/ZWe/jhYdw1e89Dr2ddzx2mxKdzUzFaZo8/mGMDpfarYqTQzmEPOSA== X-Received: by 10.236.129.194 with SMTP id h42mr3749134yhi.11.1381266197970; Tue, 08 Oct 2013 14:03:17 -0700 (PDT) Received: from corp2gmr1-1.hot.corp.google.com (corp2gmr1-1.hot.corp.google.com [172.24.189.92]) by gmr-mx.google.com with ESMTPS id k45si1900677yhn.4.1969.12.31.16.00.00 (version=TLSv1.1 cipher=AES128-SHA bits=128/128); Tue, 08 Oct 2013 14:03:17 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-1.hot.corp.google.com (Postfix) with ESMTP id 3D40231C1C9; Tue, 8 Oct 2013 14:03:17 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21076.29460.644580.365307@ruffy.mtv.corp.google.com> Date: Tue, 08 Oct 2013 21:03:00 -0000 To: Joel Brobecker Cc: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH 4/7] Move struct varobj to varobj.h. In-Reply-To: <20131008045620.GD3092@adacore.com> References: <1379512482-31773-1-git-send-email-yao@codesourcery.com> <1379512482-31773-5-git-send-email-yao@codesourcery.com> <20131002094636.GC2971@adacore.com> <525103F5.90607@codesourcery.com> <20131008045620.GD3092@adacore.com> X-IsSubscribed: yes X-SW-Source: 2013-10/txt/msg00240.txt.bz2 Joel Brobecker writes: > > How about this? > > It does look a lot better to me, FWIW. The only possibly contentious > question left would be making struct varobj public, when I personally > tend to prefer opaque structures. But I'm fine with this step, as it > helps achieve the goal of moving the language-specific stuff out of > varobj.c. I think Doug also pretty much agreed to that change. I would > give the patch, say, until the end of the week, JIC others want to > comment in. Yeah, except for the nits you found, looks good to me. [I can image more structs will get moved into headers in a c++ world, thus this doesn't bother me.] > How does this new patch affect the rest of the patch series? No effect? > If not, we can continue reviewing the remainder. Otherwise, can you > post an update? Sorry it's taking so long. I just don't have much time. > But as I said, I like the direction this is taking. > > Thanks! > > > 2013-10-06 Yao Qi > > > > * varobj.c (struct varobj): Move most of the fields to > > varobj.h. > > (struct varobj_dynamic): New struct. > > (varobj_get_display_hint) [HAVE_PYTHON]: Adjust. > > (varobj_has_more): Likewise. > > (dynamic_varobj_has_child_method): Likewise. > > (update_dynamic_varobj_children): Likewise. > > (varobj_get_num_children): Likewise. > > (varobj_list_children, varobj_pretty_printed_p): Likewise. > > (install_new_value_visualizer): Likewise. > > (install_new_value_visualizer, install_new_value): Likewise. > > (varobj_update, new_variable, free_variable): Likewise. > > (my_value_of_variable, value_get_print_value): Likewise. > > (install_visualizer): Change the type of parameter 'var' to > > 'struct varobjd_dynamic *'. Callers update. > > * varobj.h (struct varobj): Moved from varobj.c. > > (struct varobj) : New field. > > > @@ -2924,7 +2861,7 @@ value_get_print_value (struct value *value, enum varobj_display_formats format, > > #if HAVE_PYTHON > > if (gdb_python_initialized) > > { > > - PyObject *value_formatter = var->pretty_printer; > > + PyObject *value_formatter= var->dynamic->pretty_printer; > > You accidently removed a space before '='. > > > +/* Every variable in the system has a structure of this type defined > > + for it. This structure holds all information necessary to manipulate > > + a particular object variable. Members which must be freed are noted. */ > > +struct varobj > > +{ > > Not sure if there is a rule for it, or not. But I tend to prefer an > empty line between documentation and structure as well (same as with > subprograms). Add it if you agree, or else feel free to ignore. This > is just an arbitrary preference, AFAIK, and it really does not matter > much to me. The more lines of code that follow, the more I like the blank line. For something like: /* blah blah blah ... */ int foo = 42; I think the blank line is unnecessary. [even if the comment itself is several lines] But for a struct definition where more lines of code are involved, I like the blank line. [There is such a rule for function comments. Whether it was originally created because functions tend to involve several lines ... I'm not sure, but I do like it, and am glad we're now enforcing it!]