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 E6747A00C2; Thu, 3 Feb 2022 13:11:45 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7011440143; Thu, 3 Feb 2022 13:11:45 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id E43C340140 for ; Thu, 3 Feb 2022 13:11:44 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v6 20/50] pdump: remove unneeded header includes Date: Thu, 3 Feb 2022 13:11:42 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86E6E@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v6 20/50] pdump: remove unneeded header includes Thread-Index: AdgY6smgaZy8W7OmT+e0uA6RYMEf/gACMw2g 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> <20220202092836.5e1aadbb@hermes.local> <98CBD80474FA8B44BF855DF32C47DC35D86E6C@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" Cc: "Stephen Hemminger" , "Sean Morrissey" , "Reshma Pattan" , 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 > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Thursday, 3 February 2022 11.43 >=20 > On Wed, Feb 02, 2022 at 07:21:33PM +0100, Morten Br=F8rup wrote: > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > > > Sent: Wednesday, 2 February 2022 18.29 > > > > > > On Wed, 2 Feb 2022 17:03:44 +0000 > > > Bruce Richardson wrote: > > > > > > > On Wed, Feb 02, 2022 at 05:45:47PM +0100, Morten Br=F8rup 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. > > > > Yes, I did understand that. Like a forward declaration, often used = in > structures referencing each other. > > > > > > There is no copy-pasting involved and the > > > > reference does not need to change as the structure changes. > > > > I meant that the rte_mempool itself could change type, so it is no > longer a structure. Then the "copy-pasted" forward declaration will > allow the code to compile, not catching that the type has changed. > > > > The refcnt member of the mbuf structure changed over time from being > a structure (specifically rte_atomic_t, which was a typedef) to being = a > simple uint16_t. So types changing over time does happen in the real > world, also in DPDK. > > >=20 > If the type does change, then it will still be caught via compilation > error, since the corresponding .c file will have to include the full > mempool header to access the internals of the data structures. That > will > trigger a compilation failure on the function in the .c file, = requiring > the > parameter type to change from e.g. struct to union. > Overall, though, if a type does change, one would expect that a global > search replace would be used throughout the whole codebase, so I > consider > the possibilities of problems here to be very, very remote. Thank you for elaborating, Bruce. Although I agree that the risk of problems is extremely low, I still = prefer eliminating risks over reducing compile time. It boils down to different opinions about the cost/benefit analysis, and = it seems that Stephen and you outnumber me, so I'll stop complaining = about it. ;-) -Morten