Re: [RFA/commit] (Windows) remove thread notification for main thread of inferior

[Simon Marchi <simon.marchi@polymtl.ca>]

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) <CREATE_THREAD_DEBUG_EVENT>
>         <CREATE_PROCESS_DEBUG_EVENT>: Likewise.
>         <EXIT_THREAD_DEBUG_EVENT, EXIT_PROCESS_DEBUG_EVENT>: 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