From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29125 invoked by alias); 25 May 2017 10:06:41 -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 29104 invoked by uid 89); 25 May 2017 10:06:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.4 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=no version=3.3.2 spammy=GIL, gil, alas, H*RU:7800 X-HELO: mail-wm0-f54.google.com Received: from mail-wm0-f54.google.com (HELO mail-wm0-f54.google.com) (74.125.82.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 May 2017 10:06:38 +0000 Received: by mail-wm0-f54.google.com with SMTP id e127so97352088wmg.1 for ; Thu, 25 May 2017 03:06:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=jKwz3NeRVBSYlcy6S7UUYmm9jiYVrkWrKYAsvC4jIFY=; b=AYdYBOGCXgZ3984FtzZqagg77g89KOtup+f01eSG43rLuFk9Cf3mlmCWIgPd/D1BOt u8k+oDqjpyVDxLsh0JLp4TdRBdkYZBMwGkm+BZoejP+EEDhCx2llet1VNGslwZYMsEyb eZdbN/U9LXprXgUhAavKjWFwmp3rm9SNaR7+/WEK6mlZ9VuXFTsyGIanK6MMZ6+xk5Mm E49CuLbBlRNGFNmZdCO6gq2Aa5BYJ5zBIj94kBHahEd+RYR7cQ2nl7iX3EYXrxtUx/uz hgxbHJESbjC32o8qBSu9iwBBy0wF0U2fNvtz4g2jn7cq86/o04ni06Njl4kveOtvv3At HmTg== X-Gm-Message-State: AODbwcDgppr8srC0GkkUN5fwpTsVmx+hhGA2dG4FTOUlhWIi0k95x9+D a41J2ls0w/l5U3Mz9aemXg== X-Received: by 10.28.128.202 with SMTP id b193mr9821125wmd.53.1495706800210; Thu, 25 May 2017 03:06:40 -0700 (PDT) Received: from ?IPv6:2a02:c7f:ae15:7800:4685:ff:fe66:9f4? ([2a02:c7f:ae15:7800:4685:ff:fe66:9f4]) by smtp.gmail.com with ESMTPSA id h73sm851364wma.10.2017.05.25.03.06.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 25 May 2017 03:06:39 -0700 (PDT) Subject: Re: [PATCH] Fix usage of to_string() for pretty-printers with children To: Peter Linss , gdb-patches@sourceware.org References: From: Phil Muldoon Message-ID: Date: Thu, 25 May 2017 10:06:00 -0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00535.txt.bz2 On 25/05/17 03:33, Peter Linss wrote: > gdb/ChangeLog: > > * varobj.c (varobj_value_get_print_value): Call pretty-printer > to_string method for value if present even when children > method is available. > (dynamic_varobj_has_child_method) Remove unused function. > > gdb/doc/ChangeLog: > > * gdb.texinfo (Variable Objects, Result): Update description of > value to reflect to_string output. Thanks. > > -#if HAVE_PYTHON > - > -static int > -dynamic_varobj_has_child_method (const struct varobj *var) > -{ > - PyObject *printer = var->dynamic->pretty_printer; > - > - if (!gdb_python_initialized) > - return 0; > - > - gdbpy_enter_varobj enter_py (var); > - return PyObject_HasAttr (printer, gdbpy_children_cst); > -} > -#endif > - In removing the above you are removing the Python environment call which, among other things, ensures the state of the Python GIL. The replacement hunk below does not make the same call? Is this intentional? > /* A factory for creating dynamic varobj's iterators. Returns an > iterator object suitable for iterating over VAR's children. */ > > @@ -2420,11 +2405,6 @@ varobj_value_get_print_value (struct value *value, > > if (value_formatter) > { > - /* First check to see if we have any children at all. If so, > - we simply return {...}. */ > - if (dynamic_varobj_has_child_method (var)) > - return "{...}"; > - > if (PyObject_HasAttr (value_formatter, gdbpy_to_string_cst)) > { > struct value *replacement; > @@ -2486,6 +2466,13 @@ varobj_value_get_print_value (struct value *value, > if (replacement) > value = replacement; > } > + else > + { > + /* If we don't have to_string but we have children, > + we simply return {...}. */ > + if (PyObject_HasAttr (value_formatter, gdbpy_children_cst)) > + return "{...}"; > + } You've removed a function but replaced it with an if (...) (and the associated safety calls). I'm not sure this is the right thing to do. Also, if the to_string call has changed, it might constitute an API change. And, alas, all changes need tests, unless obvious, and the outcome of this test is not obvious (well to me ;) ) Cheers Phil