From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 63418 invoked by alias); 23 May 2015 15:29:23 -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 63408 invoked by uid 89); 23 May 2015 15:29:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sat, 23 May 2015 15:29:12 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4NFTA68009293 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Sat, 23 May 2015 11:29:10 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4NFT8a6031784; Sat, 23 May 2015 11:29:09 -0400 Message-ID: <55609CC4.1050506@redhat.com> Date: Sat, 23 May 2015 15:29:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Doug Evans CC: gdb-patches@sourceware.org Subject: Re: [PATCH v3 09/17] Teach non-stop to do in-line step-overs (stop all, step, restart) References: <001a11c2e4b2a5e2c60516b0d286@google.com> In-Reply-To: <001a11c2e4b2a5e2c60516b0d286@google.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-05/txt/msg00625.txt.bz2 On 05/22/2015 08:39 PM, Doug Evans wrote: > Pedro Alves writes: > > > Can you elaborate on how to interpret the name of this function? > > > Guessing at how I'm supposed to interpret what this function is for, > > > is a better name > > >> "breakpoints_should_have_been_inserted_by_now_or_should_remain_inserted"? > > > [Not that that's my recommendation :-). Just trying to understand how > > > to read this function.] > > > > You got it right, but I'm afraid I lack the English skills to come > > up with a better name. "be inserted" is the state we want to reach, not > > an action. Maybe "should_breakpoints_be_inserted_now" is little clearer, > > but it still doesn't distinguish "state" vs "action". Because > > state("be inserted"=false) is clearly "no breakpoints on target", > > while action("be inserted"=false) could mean that whatever > > breakpoint is already inserted remains inserted. > > I think I can manage with this change: > > - /* Don't remove breakpoints yet if, even though all threads are > - stopped, we still have events to process. */ > + /* We still need breakpoints, even though all threads are > + stopped, if we still have events to process. */ > I like that. Thanks. > But I'll submit that separately to not interfere with > this patchset. I don't mind folding that in at all. Just let me know. For better names, your version of the comment made me think of: breakpoints_should_be_on_target breakpoints_should_be_on_target_now need_breakpoint_on_target breakpoints_needed_on_target need_breakpoint_on_target_now breakpoints_needed_on_target_now want_breakpoints_on_target breakpoints_wanted_on_target want_breakpoints_on_target_now breakpoints_wanted_on_target_now Main difference is "be on target" instead of "be inserted". The former seems less ambiguous to me. > > > > > + if (tp->suspend.waitstatus_pending_p) > > > > + { > > > > + if (debug_infrun) > > > > + { > > > > + char *statstr; > > > > + > > > > + statstr = target_waitstatus_to_string (&tp->suspend.waitstatus); > > > > + fprintf_unfiltered (gdb_stdlog, > > > > + "infrun: resume: thread %s has pending wait status %s " > > > > + "(currently_stepping=%d).\n", > > > > + target_pid_to_str (tp->ptid), statstr, > > > > + currently_stepping (tp)); > > > > + xfree (statstr); > > > > > > Not something that has to be done with this patch of course, > > > but it's nice that we don't have to track the memory of > target_pid_to_str; > > > IWBN to be able to do the same for target_waitstatus_to_string. > > > [In C++ it could just return a string, and we *could* just wait for > C++. > > > Just a thought.] > > > > I'm just going with the flow you yourself created. ;-) > > What's the point of saying something like that? Sorry, that was uncalled for. Too many late hours lead to bad jokes. Please accept my apologies. > I'm going to assume you don't disagree with the improvement. I don't. Thanks, Pedro Alves