RE: [PATCH v6 20/50] pdump: remove unneeded header includes

["Morten Brørup" <mb@smartsharesystems.com>]

> 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 <sean.morrissey@intel.com> wrote:
> >
> > > These header includes have been flagged by the iwyu_tool
> > > and removed.
> > >
> > > Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> > > ---

[...]

> >
> > > 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 <stdint.h>
> > > -#include <rte_mempool.h>
> > > -#include <rte_ring.h>
> > >  #include <rte_bpf.h>
> > >
> > >  #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.

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.

> 
> > 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.