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

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Feb 02, 2022 at 07:21:33PM +0100, Morten Brørup 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 <bruce.richardson@intel.com> wrote:
> > 
> > > 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 <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.
> > >
> > > 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.
> 

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.

/Bruce