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 CEE6BA00C5; Wed, 2 Feb 2022 17:45:50 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 46D5A410D5; Wed, 2 Feb 2022 17:45:50 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 929C740E28 for ; Wed, 2 Feb 2022 17:45:49 +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: Wed, 2 Feb 2022 17:45:47 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86E6B@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: AdgYTgvdlRh7nmf2SQmBb8HP6vM6dgAA9Cwg 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> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" , "Stephen Hemminger" Cc: "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: Wednesday, 2 February 2022 17.01 >=20 > 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. >=20 > 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. If only using one function from a library, you probably wouldn't copy = the function prototype instead of including the library header file. Let's focus on the speed of compiled DPDK code, not the speed of = compiling DPDK code. Code readability and lower probability of = introducing bugs is far more important than compilation time! Cleaning up code is also good, so the iwyu_tool initiative is still = good. >=20 > > Since this is an API header, changing it here risks breaking an > application. > > > Good point. Should we avoid removing headers from public headers in > case of > application breakage? It's safer, but it means that we will likely > still be > including far too many headers in multiple places. If we do remove > them, it > would not be an ABI break, just an API one, of sorts. The application only breaks at compile time, and it should be easy for = the application developer to see what is missing. I vote for removing unused headers in public files too - not considering = it an API breakage.