From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 32FA2A00C5; Wed, 2 Feb 2022 18:19:54 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9F605410FF; Wed, 2 Feb 2022 18:19:53 +0100 (CET) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mails.dpdk.org (Postfix) with ESMTP id 4D569410DC for ; Wed, 2 Feb 2022 18:19:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643822392; x=1675358392; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=gyUyoLKU6MJumhy8CiTOfCwc/LoRdkiCFhfXnHTtj6o=; b=HVifO4pqzqczjadxET6RFmKqwmcGgUwr5eaOaJ7FgHYq5o+7GOR85gDe HCRgxWBmBi/6pK0H18pVb1XqVtqbP3krX3hT9X400u/FersErROUxOekr lbtxTatTtuK1On9BpTCd9C0KvYcYgU6Ny1RBAIhRx9LSQZIV2agvxwwft z5WIY80faZoC/A3ntFOWMJtdkl5utMtE0ql0n1Ryo0TP3/BkkBwA780DE UJH+QbpDMQ5D8GD3vlpWorq854aV2zcdQMB2KFFxAXrGSpOGkR1wKoy09 K3wUXBu+dGyXNAWULM4wtvXpB+hxGhPuExIWfHuhk5kpWkxX55LV71SYY A==; X-IronPort-AV: E=McAfee;i="6200,9189,10246"; a="228629649" X-IronPort-AV: E=Sophos;i="5.88,337,1635231600"; d="scan'208";a="228629649" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Feb 2022 09:03:49 -0800 X-IronPort-AV: E=Sophos;i="5.88,337,1635231600"; d="scan'208";a="698962197" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.31.254]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 02 Feb 2022 09:03:47 -0800 Date: Wed, 2 Feb 2022 17:03:44 +0000 From: Bruce Richardson To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Stephen Hemminger , Sean Morrissey , Reshma Pattan , dev@dpdk.org Subject: Re: [PATCH v6 20/50] pdump: remove unneeded header includes Message-ID: References: <20220117201943.873922-1-sean.morrissey@intel.com> <20220202094802.3618978-1-sean.morrissey@intel.com> <20220202094802.3618978-21-sean.morrissey@intel.com> <20220202075458.3f8cd2c1@hermes.local> <98CBD80474FA8B44BF855DF32C47DC35D86E6B@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D86E6B@smartserver.smartshare.dk> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Feb 02, 2022 at 05:45:47PM +0100, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > Sent: Wednesday, 2 February 2022 17.01 > > > > On Wed, Feb 02, 2022 at 07:54:58AM -0800, Stephen Hemminger wrote: > > > On Wed, 2 Feb 2022 09:47:32 +0000 > > > Sean Morrissey wrote: > > > > > > > These header includes have been flagged by the iwyu_tool > > > > and removed. > > > > > > > > Signed-off-by: Sean Morrissey > > > > --- > > [...] > > > > > > > > diff --git a/lib/pdump/rte_pdump.h b/lib/pdump/rte_pdump.h > > > > index 6efa0274f2..41c4b7800b 100644 > > > > --- a/lib/pdump/rte_pdump.h > > > > +++ b/lib/pdump/rte_pdump.h > > > > @@ -13,8 +13,6 @@ > > > > */ > > > > > > > > #include > > > > -#include > > > > -#include > > > > #include > > > > > > > > #ifdef __cplusplus > > > > > > This header does use rte_mempool and rte_ring in rte_pdump_enable(). > > > Not sure why IWYU thinks they should be removed. > > > > Because they are only used as pointer types, not as structures > > themselves. > > Normally in cases like this, I would put in just "struct rte_mempool;" > > at > > the top of the file rather than including a whole header just for one > > structure. > > I don't think we should introduce such a hack! > If a module uses something from a library, it makes sense to include the header file for the library. > > Putting in "struct rte_mempool;" is essentially copy-pasting from the library, although only a structure. What happens if the type changes or disappears, or depends on some #ifdef? It could have one type in some cases and another type in other cases - e.g. the atomic counters in the mbuf once had different types, depending on compile time flags. The copy-pasted code would not get fixed if the type evolved over time. By "struct rte_mempool;" I mean literally just that. All it does is indicate that there is a structure defined somewhere else that will be used via pointer in the file later on. There is no copy-pasting involved and the reference does not need to change as the structure changes. >From what I read, having this forward declaration is not necessary for C, but for C++ if you use the struct pointer in a function definition later on, you may get an error. Therefore, if you are using a struct only as a pointer parameter, the best option is to forward declare it (to keep C++ happy), and not include a whole header file unnecessarily. /Bruce