From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95440 invoked by alias); 12 Feb 2019 14:00:58 -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 95429 invoked by uid 89); 12 Feb 2019 14:00:58 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:3271, justification 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; Tue, 12 Feb 2019 14:00:52 +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 x1CE0kh2024754 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 12 Feb 2019 09:00:50 -0500 Received: by simark.ca (Postfix, from userid 112) id F2EFD1E650; Tue, 12 Feb 2019 09:00:45 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id B917F1E152; Tue, 12 Feb 2019 09:00:43 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 12 Feb 2019 14:00:00 -0000 From: Simon Marchi To: Joel Brobecker Cc: gdb-patches@sourceware.org Subject: Re: [RFA/commit] (Windows) remove thread notification for main thread of inferior In-Reply-To: <20190212112529.GA8317@adacore.com> References: <20190210132204.6139-1-brobecker@adacore.com> <72558bcdf9f15fa3d1c569cc1c689996@polymtl.ca> <20190212112529.GA8317@adacore.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg00136.txt.bz2 On 2019-02-12 06:25, Joel Brobecker wrote: > Hi Simon, > >> Hi Joel, >> >> I didn't test, but this looks good to me. Two small comments below. > > Thanks for the review! > >> > diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c >> > index 2894b208f58..ae05d889a6a 100644 >> > --- a/gdb/windows-nat.c >> > +++ b/gdb/windows-nat.c >> > @@ -426,9 +426,17 @@ thread_rec (DWORD id, int get_context) >> > return NULL; >> > } >> > >> > -/* Add a thread to the thread list. */ >> > +/* Add a thread to the thread list. >> > + >> > + PTID is the ptid of the thread to be deleted. >> > + H is its Windows handle. >> > + TLB is its thread local base. >> > + MAIN_THREAD_P should be true if the thread to be deleted is >> > + the main thread, false otherwise. */ >> >> This comment about the function that adds threads talks about things >> to be >> deleted. > > Indeed. Fixed in the attached version. > >> > static windows_thread_info * >> > -windows_add_thread (ptid_t ptid, HANDLE h, void *tlb) >> > +windows_add_thread (ptid_t ptid, HANDLE h, void *tlb, >> > + bool main_thread_p = false) >> >> Just a nit: in this case, where there are very few callers to update, >> I >> would opt for not using a default parameter value. It's probably just >> a >> personal preference, but I find it clearer to have the explicit value >> at the >> call site. > > It wasn't really the number of callers to update that prompted me to > use a default value, but rather the fact that, in the normal case, > that parameter is really redundant, at least as far as the reader's > understanding is concerned. This is related a bit to the fact that > I am partial to the option of being able to name parameters at the > point call, something that C++ doesn't support. This would have been > particularly useful in this case here, because I've always found > "true" or "false" litteral constants in function calls like that > to be rather mysterious. So much so that I felt I needed to add > a comment to name them that way. Since it felt like providing it > at the point of call when false was redundant, I decided against > providing its value. > > The attached patch follows your suggestion nonetheless. I worked > around my concern the same way I did when passing the new parameter > as true, and so this is good for me. Your rationale makes sense, I am fine with your original code too since you gave a good justification. However you prefer. I agree about the fact that literal values in function calls are often not that readable, and not only true/false. > gdb/ChangeLog > > * windows-nat.c (windows_add_thread): Add new parameter > "main_thread_p" with default value set to false. Update > function documentation as well as all callers. > (windows_delete_thread): Likewise. > (fake_create_process): Update call to windows_add_thread. > (get_windows_debug_event) > : Likewise. > : Update > call to windows_delete_thread. > > Tested on x86-windows (MinGW) using AdaCore's testsuite. > OK to push? > > Thanks! There is still a mention of "deleted" in the windows_add_thread doc, otherwise LGTM. Simon