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

Simon Marchi
2019-02-12T14:00:58Z

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