From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47196 invoked by alias); 8 Dec 2017 18:42:31 -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 47100 invoked by uid 89); 8 Dec 2017 18:42:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 08 Dec 2017 18:42:28 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id vB8IgLwv009059 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 8 Dec 2017 13:42:26 -0500 Received: by simark.ca (Postfix, from userid 112) id 7EF841E586; Fri, 8 Dec 2017 13:42:21 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 367E71E02D; Fri, 8 Dec 2017 13:42:20 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 08 Dec 2017 18:42:00 -0000 From: Simon Marchi To: Pedro Alves Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] python: Add qualified parameter to gdb.Breakpoint In-Reply-To: <18e86b00-7659-e57f-e650-fae62ebf1d2c@redhat.com> References: <1512686013-24658-1-git-send-email-simon.marchi@ericsson.com> <18e86b00-7659-e57f-e650-fae62ebf1d2c@redhat.com> Message-ID: <7f59352579a659f3ee96f6a9e5404f9b@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.2 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 8 Dec 2017 18:42:21 +0000 X-IsSubscribed: yes X-SW-Source: 2017-12/txt/msg00196.txt.bz2 On 2017-12-08 07:05, Pedro Alves wrote: > That was fast. :-) Thanks! > > See comments about the docs below. > > The code looks fine to me, except a formatting nit. > >> --- a/gdb/doc/python.texi >> +++ b/gdb/doc/python.texi >> @@ -4885,7 +4885,7 @@ create both breakpoints and watchpoints. The >> second accepts separate Python >> arguments similar to @ref{Explicit Locations} and can only be used to >> create >> breakpoints. >> >> -@defun Breakpoint.__init__ (spec @r{[}, type @r{][}, wp_class @r{][}, >> internal @r{][}, temporary @r{]}) >> +@defun Breakpoint.__init__ (spec @r{[}, type @r{][}, wp_class @r{][}, >> internal @r{][}, temporary @r{][}, qualified @r{]}) >> Create a new breakpoint according to @var{spec}, which is a string >> naming the >> location of a breakpoint, or an expression that defines a watchpoint. >> The >> contents can be any location recognized by the @code{break} command >> or, in the >> @@ -4909,14 +4909,20 @@ The optional @var{temporary} argument makes >> the breakpoint a temporary >> breakpoint. Temporary breakpoints are deleted after they have been >> hit. Any >> further access to the Python breakpoint after it has been hit will >> result in a >> runtime error (as that breakpoint has now been automatically >> deleted). >> + >> +The optional @var{qualified} argument is a boolean that allows >> restricting the >> +breakpoint to free-functions. > > "free-functions" is incorrect. With: > > struct A { void func (); } // #1 > namespace B { struct A { void func (); } } // #2 > > "b -q A::func()" only matches #1 while > "b A::func()" matches both #1 and #2. > > and A::func() above is not a free function. > > Here's what we say for -qualified in the explicit locations part of the > manual: > > ~~~ > @item -qualified > > This flag makes @value{GDBN} interpret a function name specified with > @kbd{-function} as a complete fully-qualified name. > > For example, assuming a C@t{++} program with symbols named > @code{A::B::func} and @code{B::func}, the @w{@kbd{break -qualified > -function B::func}} command sets a breakpoint on @code{B::func}, only. > > (Note: the @kbd{-qualified} option can precede a linespec as well > (@pxref{Linespec Locations}), so the particular example above could be > simplified as @w{@kbd{break -qualified B::func}}.) > ~~~ > > There's similar text in the linespecs part of the manual and also > in "help break". See: > $ git show a20714ff39f6 -- doc/gdb.texinfo NEWS break.c > > So we either need to clarify that in the Python bits too, > or some do some xref'ing. Agreed, I didn't like that wording either. I had only found the Linespec Locations page, which doesn't specifically say "fully-qualified name", but mentions free-function (though it's in an example, not a formal definition). How is this? The optional @var{qualified} argument is a boolean that allows interpreting the function passed in @code{spec} as a fully-qualified name. It is equivalent to @code{break}'s @code{-qualified} flag (@pxref{Linespec Locations} and @ref{Explicit Locations}). > It is equivalent to @code{break}'s >> +@code{-qualified} flag (@pxref{Linespec Locations}). >> + >> @end defun >> >> -@defun Breakpoint.__init__ (@r{[} source @r{][}, function @r{][}, >> label @r{][}, line @r{]}, @r{][} internal @r{][}, temporary @r{][}) >> +@defun Breakpoint.__init__ (@r{[} source @r{][}, function @r{][}, >> label @r{][}, line @r{]}, @r{][} internal @r{][}, temporary @r{][}, >> qualified @r{]}) >> Create a new explicit location breakpoint (@pxref{Explicit >> Locations}) >> according to the specifications contained in the key words >> @var{source}, >> @var{function}, @var{label} and @var{line}. > > Should @var{qualified} be added to this list too? It is mentioned in the paragraph below. > (BTW, noticed a typo in this paragraph: > > "create a new a explicit location" > > the second "a" is spurious.) That text changed based on Eli's comments, but thanks anyway. >> >> -@var{internal} and @var{temporary} have the same usage as explained >> previously. >> +@var{internal}, @var{temporary} and @var{qualified} have the same >> usage as >> +explained previously. >> @end defun >> >> The available types are represented by constants defined in the >> @code{gdb} > >> @@ -759,6 +761,9 @@ bppy_init (PyObject *self, PyObject *args, >> PyObject *kwargs) >> case bp_breakpoint: >> { >> event_location_up location; >> + symbol_name_match_type func_name_match_type >> + = qualified ? symbol_name_match_type::FULL >> + : symbol_name_match_type::WILD; > > Should wrap the multi-line expression in ()s: > > symbol_name_match_type func_name_match_type > = (qualified ? symbol_name_match_type::FULL > : symbol_name_match_type::WILD); > > Though I prefer breaking ternary operators like this: > > symbol_name_match_type func_name_match_type > = (qualified > ? symbol_name_match_type::FULL > : symbol_name_match_type::WILD); > > because it makes it look more obviously like > > if COND > THEN > ELSE Done. Thanks, Simon