From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id FWZ+I17KkmWDLDAAWB0awg (envelope-from ) for ; Mon, 01 Jan 2024 09:21:18 -0500 Authentication-Results: simark.ca; dkim=pass (2048-bit key; secure) header.d=lancelotsix.com header.i=@lancelotsix.com header.a=rsa-sha256 header.s=2021 header.b=pGr8Nbsy; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 82F3A1E0C3; Mon, 1 Jan 2024 09:21:18 -0500 (EST) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 118191E0B9 for ; Mon, 1 Jan 2024 09:21:16 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5D12C3858415 for ; Mon, 1 Jan 2024 14:21:15 +0000 (GMT) Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id D810B3858D1E for ; Mon, 1 Jan 2024 14:20:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D810B3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org D810B3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=51.195.220.111 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704118851; cv=none; b=HMYpRPDJkw+Ws16v/bAwmAEr6j1l+p2DQF3lKp4OvU/7rfItHoVYrYt/VDgy7DLjEcKUSkhHEap8AGxnfa+ULaMylW5m49EzXQXDoUzrFz6MrWigx9O0qWuyeJCflea4uNm4WiauUNHU1AeAVQOBRRb34Vebsvx9ykXEwlyRafM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704118851; c=relaxed/simple; bh=oHN+GLDtHji/F8/gQ1V3jTELcf+8LuJ0pFeM5XYTdew=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=asnbYUkb3flJsIHYv8cRd797a+xsvnVG/rymwGYpvSgrS7hdfhc9ubLisEhPUflCgK5XmthT2EtzXXlQGT7Jx5hUZ0Wwt0zYcSUwGGcSi6KiocfKRVbdb9dxY0jRkNJEWu/qJgywxE+rLJ7gr2lA4rR9qdmCR6DzQpW/J+9Aogo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from octopus (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id D2D5F8115E; Mon, 1 Jan 2024 14:20:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=lancelotsix.com; s=2021; t=1704118847; bh=oHN+GLDtHji/F8/gQ1V3jTELcf+8LuJ0pFeM5XYTdew=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=pGr8Nbsyu68BQ/KRNaAIkFMRmftE3I1Z3slsiZkc1TVdyvcgyHfK6lHsuI44WRzzd PbHWBy4aqd0t1BblMmzhmkAX6DXriADillmLNszcs3BvHtNttPkZi1yFTG53d14wy6 tgAx/mZ20KnkgyNeIfSWYWCZuCK4BrnfA17NFsY/Nyh0N7/OsY0oYrXoYenHVuP1bc L0j6jvp3PCgPbwOolB9hH61JA3+F9VpWLlYo+pLRMIeF8g0WRXKkCbO+QLDQU0d3+Q tv/STwmUP0iWIqMmi9nyGAMQXA8OwwqDCV2B6X/1GSo0UguE8X4Onm+DZYaW1IINwe 55S9tV6EDEIfQ== Date: Mon, 1 Jan 2024 14:20:41 +0000 From: Lancelot SIX To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/6] Use vector in remote-fileio.c Message-ID: <20240101142041.dndg63bv42hs7lpo@octopus> References: <20231231-remote-fileio-v1-0-249cc6c440d9@tromey.com> <20231231-remote-fileio-v1-2-249cc6c440d9@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20231231-remote-fileio-v1-2-249cc6c440d9@tromey.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.6.2 (lndn.lancelotsix.com [0.0.0.0]); Mon, 01 Jan 2024 14:20:47 +0000 (UTC) X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Hi Tom, I have a couple of comments below. On Sun, Dec 31, 2023 at 01:25:39PM -0700, Tom Tromey wrote: > This changes remote_fio_data to be an object holding a vector. This > simplifies the code somewhat. > --- > gdb/remote-fileio.c | 51 +++++++++++++++++---------------------------------- > 1 file changed, 17 insertions(+), 34 deletions(-) > > diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c > index 367edb54f74..4891b0c5b92 100644 > --- a/gdb/remote-fileio.c > +++ b/gdb/remote-fileio.c > @@ -37,9 +37,9 @@ > #endif > #include > > -static struct { > - int *fd_map; > - int fd_map_size; > +static struct > +{ > + std::vector fd_map; > } remote_fio_data; > > #define FIO_FD_INVALID -1 > @@ -51,16 +51,13 @@ static int remote_fio_system_call_allowed = 0; > static int > remote_fileio_init_fd_map (void) > { > - int i; > - > - if (!remote_fio_data.fd_map) > + if (remote_fio_data.fd_map.empty ()) > { > - remote_fio_data.fd_map = XNEWVEC (int, 10); > - remote_fio_data.fd_map_size = 10; > + remote_fio_data.fd_map.resize (10); > remote_fio_data.fd_map[0] = FIO_FD_CONSOLE_IN; > remote_fio_data.fd_map[1] = FIO_FD_CONSOLE_OUT; > remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT; > - for (i = 3; i < 10; ++i) > + for (int i = 3; i < 10; ++i) > remote_fio_data.fd_map[i] = FIO_FD_INVALID; std::vector::resize has an overload which takes a value to use for new elements of the vector. This could be simplified as remote_fio_data.fd_map.resize (10, FIO_FD_INVALID); remote_fio_data.fd_map[0] = FIO_FD_CONSOLE_IN; remote_fio_data.fd_map[1] = FIO_FD_CONSOLE_OUT; remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT; See below, but overall, I don’t really think we need to keep the "grow by 10" strategy when using std::vector, this could simply be remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_IN) remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_OUT) remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_OUT) > } > return 3; > @@ -69,25 +66,20 @@ remote_fileio_init_fd_map (void) > static int > remote_fileio_resize_fd_map (void) > { > - int i = remote_fio_data.fd_map_size; > - > - if (!remote_fio_data.fd_map) > + if (remote_fio_data.fd_map.empty ()) > return remote_fileio_init_fd_map (); > - remote_fio_data.fd_map_size += 10; > - remote_fio_data.fd_map = > - (int *) xrealloc (remote_fio_data.fd_map, > - remote_fio_data.fd_map_size * sizeof (int)); > - for (; i < remote_fio_data.fd_map_size; i++) > + > + int i = remote_fio_data.fd_map.size (); > + remote_fio_data.fd_map.resize (i + 10); > + for (; i < remote_fio_data.fd_map.size (); i++) > remote_fio_data.fd_map[i] = FIO_FD_INVALID; Similarly, if you keep the "grow by 10" strategy this could be remote_fio_data.fd_map.resize (i + 10, FIO_FD_INVALID); > - return remote_fio_data.fd_map_size - 10; > + return remote_fio_data.fd_map.size () - 10; > } > > static int > remote_fileio_next_free_fd (void) > { > - int i; > - > - for (i = 0; i < remote_fio_data.fd_map_size; ++i) > + for (int i = 0; i < remote_fio_data.fd_map.size (); ++i) > if (remote_fio_data.fd_map[i] == FIO_FD_INVALID) > return i; I think that’s mostly a personal preference thing, but std::find could be used instead of an explicit loop auto it = std::find (remote_fio_data.fd_map.begin (), remote_fio_data.fd_map.end (), FIO_FD_INVALID); if (it != remote_fio_data.fd_map.end ()) return std::distance (remote_fio_data.fd_map.begin (), it); return remote_fileio_resize_fd_map (); I think it could be possible to go one step further and remove remote_fileio_resize_fd_map entirely. The "grow by 10" strategy is not necessary anymore if you use a std::vector. A similar allocation strategies comes for free. static int remote_fileio_next_free_fd () { auto it = std::find (remote_fio_data.fd_map.begin (), remote_fio_data.fd_map.end (), FIO_FD_INVALID); if (it != remote_fio_data.fd_map.end ()) return std::ditance (remote_fio_data.fd_map.begin (), it); remote_fio_data.fd_map.emplace_back (FIO_FD_INVALID); return remote_fio_data.fd_map.size () - 1; } One could also inline this inside remote_fileio_map_fd, and directly set the appropriate FD value instead of using FIO_FD_INVALID temporarily. But I would be happy keeping things this way for readability. Best, Lancelot. > return remote_fileio_resize_fd_map (); > @@ -106,7 +98,7 @@ static int > remote_fileio_map_fd (int target_fd) > { > remote_fileio_init_fd_map (); > - if (target_fd < 0 || target_fd >= remote_fio_data.fd_map_size) > + if (target_fd < 0 || target_fd >= remote_fio_data.fd_map.size ()) > return FIO_FD_INVALID; > return remote_fio_data.fd_map[target_fd]; > } > @@ -115,7 +107,7 @@ static void > remote_fileio_close_target_fd (int target_fd) > { > remote_fileio_init_fd_map (); > - if (target_fd >= 0 && target_fd < remote_fio_data.fd_map_size) > + if (target_fd >= 0 && target_fd < remote_fio_data.fd_map.size ()) > remote_fio_data.fd_map[target_fd] = FIO_FD_INVALID; > } > > @@ -1147,21 +1139,12 @@ do_remote_fileio_request (remote_target *remote, char *buf) > void > remote_fileio_reset (void) > { > - int ix; > - > - for (ix = 0; ix != remote_fio_data.fd_map_size; ix++) > + for (int fd : remote_fio_data.fd_map) > { > - int fd = remote_fio_data.fd_map[ix]; > - > if (fd >= 0) > close (fd); > } > - if (remote_fio_data.fd_map) > - { > - xfree (remote_fio_data.fd_map); > - remote_fio_data.fd_map = NULL; > - remote_fio_data.fd_map_size = 0; > - } > + remote_fio_data.fd_map.clear (); > } > > /* Handle a file I/O request. BUF points to the packet containing the > > -- > 2.43.0 >