From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id A1854385801A for ; Sun, 23 May 2021 01:08:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A1854385801A 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 14N18JbN031497 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sat, 22 May 2021 21:08:24 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 14N18JbN031497 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id E9CE31E813; Sat, 22 May 2021 21:08:18 -0400 (EDT) Subject: Re: [PATCH] gdb: make iterate_over_threads a template function To: Andrew Burgess , gdb-patches@sourceware.org References: <20210522165146.4113176-1-andrew.burgess@embecosm.com> From: Simon Marchi Message-ID: <669152ee-86c6-86ec-1b4c-2b06a115f1d6@polymtl.ca> Date: Sat, 22 May 2021 21:08:18 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20210522165146.4113176-1-andrew.burgess@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sun, 23 May 2021 01:08:19 +0000 X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 May 2021 01:08:33 -0000 On 2021-05-22 12:51 p.m., Andrew Burgess wrote:> This started off as changing the return type of the callback function > that iterate_over_threads takes from int to bool. > > But then I thought, why not make the whole thing templated, and remove > the need to pass void* arguments around. > > So now we can do: > > bool > callback_1 (struct thread_info *tp) > { ... } > > bool > callback_2 (struct thread_info *tp, const struct some_type &obj) > { ... } > > void > some_other_part_of_gdb () > { > iterate_over_threads (callback_1); > > iterate_over_threads (callback_2, object_of_some_type); > } > > Which seems much nicer to me. > > While updating all the callback functions I took this opportunity to > remove a code construct I really hate: > > if (condition) > return true; > return false; > > This has now become: > > return (condition); > > There should be no user visible changes after this commit. I kind of recall that when I introduced the find_thread / for_each_thread functions in GDBserver, I had done the way you did, but Pedro suggested to just have a single Func parameter (like it is now), and that we could use lambdas if needed. And that the syntax for_each_thread (my_callback, some, args); ... doesn't make it super clear what's happening. Versus: for_each_thread ([captures] (thread_info *thread) { my_callback (thread, some, args); }); ... which makes it look kind of like a regular for loop. But speaking of for loops, the most readable and straightforward of all would probably be to just use range-for loop directly. Instead of writing: iterate_over_threads (proceed_thread_callback, pid); write: for (thread_info *thread : all_threads_safe ()) proceed_thread (thread, pid); And if it's easy to determine that the for loop body won't delete the thread, use all_threads instead of all_threads_safe (I wouldn't be suprised if the cost of using all_threads_safe over all_threads was insignificant, so perhaps we could have only all_threads, and make it safe by default). For the cases where iterate_over_threads is used to find a thread (i.e. the callback doesn't always return false), then it could also be transformed in a ranged-for plus an "if" plus a "break". But that just makes the calling code more complex, I don't suggest that. I would instead suggest to keep using iterate_over_threads, but rename it find_thread. find_thread speaks more about what the intent is, whereas iterate_over_threads speaks about the mean. So something like: t = find_thread ([captures] (thread_info *thread) { return my_predicate (thread); }; Having find_thread in GDB would align it with how it is in GDBserver. And I think we could get rid of for_each_thread in GDBserver in favor of range-based for loops. Simon