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

[Bruce Richardson]

On Wed, Feb 02, 2022 at 05:45:47PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson at 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 at intel.com> wrote:
> > >
> > > > These header includes have been flagged by the iwyu_tool
> > > > and removed.
> > > >
> > > > Signed-off-by: Sean Morrissey <sean.morrissey at 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.

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