This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]