From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 72967 invoked by alias); 17 Feb 2019 14:57:18 -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 72937 invoked by uid 89); 17 Feb 2019 14:57:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Nice, cleanups, sk:get_ada, sk:iterate X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 17 Feb 2019 14:57:15 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id CFC275605C; Sun, 17 Feb 2019 09:57:13 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id C5+O73PuxzVn; Sun, 17 Feb 2019 09:57:13 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 67E825605B; Sun, 17 Feb 2019 09:57:13 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id DA089838C7; Sun, 17 Feb 2019 18:57:08 +0400 (+04) Date: Sun, 17 Feb 2019 14:57:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Minor Ada task cleanups Message-ID: <20190217145708.GC8537@adacore.com> References: <20190215212253.25409-1-tromey@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190215212253.25409-1-tromey@adacore.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-SW-Source: 2019-02/txt/msg00270.txt.bz2 Hi Tom, On Fri, Feb 15, 2019 at 02:22:53PM -0700, Tom Tromey wrote: > While working on the Ada task code, I noticed a few things that could > be cleaned up: > > * task_list_valid_p was not set in all cases in ada_build_task_list. > This causes many needless re-fetches of the task list. > > * task_list_valid_p can be bool, and various functions can also return > bool. > > * Nothing checks the return value of read_known_tasks, so it can be > changed to return void. > > * The call to ada_build_task_list in > ravenscar_thread_target::update_thread_list is redundant, because > this is the first thing done by iterate_over_live_ada_tasks. > > Tested using the internal AdaCore test suite against a ravenscar > target. Thanks for doing this. Nice cleanup :). One question independent of this patch first: I am wondering - how do you deal with submissions that you want to "git am" but have a ChangeLog patch in it that cause a merge failure? I've needed to apply your patch a few times, and the only option I knew was to remove the hungs for the ChangeLog files. > gdb/ChangeLog > 2019-02-15 Tom Tromey > > * ravenscar-thread.c > (ravenscar_thread_target::update_thread_list): Don't call > ada_build_task_list. > * ada-lang.h (ada_build_task_list): Don't declare. > * ada-tasks.c (struct ada_tasks_inferior_data) > : Now bool. > (read_known_tasks, ada_task_list_changed) > (ada_tasks_invalidate_inferior_data): Update. > (read_known_tasks_array): Return bool. > (read_known_tasks_list): Likewise. > (read_known_tasks): Return void. > (ada_build_task_list): Now static. Since this code is used on all platforms and native platforms in particular have a lot more testcases that exercise this code, it would be nice to exercise it both with the build-bot and AdaCore's testsuite. One minor nit below. Other than that, the patch looks good to me; if the patch with the change I suggested passes testing, it's OK to push. Thank you! > +static void > +read_known_tasks () > { > struct ada_tasks_inferior_data *data = > get_ada_tasks_inferior_data (current_inferior ()); > @@ -965,29 +966,30 @@ read_known_tasks (void) > ada_tasks_inferior_data_sniffer (data); > gdb_assert (data->known_tasks_kind != ADA_TASKS_UNKNOWN); > > + /* Step 3: Set task_list_valid_p, to avoid re-reading the Known_Tasks > + array unless needed. */ > switch (data->known_tasks_kind) > { > - case ADA_TASKS_NOT_FOUND: /* Tasking not in use in inferior. */ > - return 0; > - case ADA_TASKS_ARRAY: > - return read_known_tasks_array (data); > - case ADA_TASKS_LIST: > - return read_known_tasks_list (data); > + case ADA_TASKS_NOT_FOUND: /* Tasking not in use in inferior. */ > + break; > + case ADA_TASKS_ARRAY: > + data->task_list_valid_p = read_known_tasks_array (data); > + break; > + case ADA_TASKS_LIST: > + data->task_list_valid_p = read_known_tasks_list (data); > + break; > + case ADA_TASKS_UNKNOWN: > + data->task_list_valid_p = true; > + break; The last case is unnecessary, as we have just asserted above that data->known_tasks_kind != ADA_TASKS_UNKNOWN. -- Joel