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

  These header includes have been flagged by the iwyu_tool
and removed.

Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
---
 lib/pdump/rte_pdump.c | 1 -
 lib/pdump/rte_pdump.h | 2 --
 2 files changed, 3 deletions(-)
  

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>
> ---
>  lib/pdump/rte_pdump.c | 1 -
>  lib/pdump/rte_pdump.h | 2 --
>  2 files changed, 3 deletions(-)
> 
> diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
> index af450695ec..b3a62df591 100644
> --- a/lib/pdump/rte_pdump.c
> +++ b/lib/pdump/rte_pdump.c
> @@ -2,7 +2,6 @@
>   * Copyright(c) 2016-2018 Intel Corporation
>   */
>  
> -#include <rte_memcpy.h>
>  #include <rte_mbuf.h>
>  #include <rte_ethdev.h>
>  #include <rte_lcore.h>

Yes, this code doesn't use rte_memcpy so yes, remove it.

> 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.
Since this is an API header, changing it here risks breaking an application.
  

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>
> > ---
> >  lib/pdump/rte_pdump.c | 1 -
> >  lib/pdump/rte_pdump.h | 2 --
> >  2 files changed, 3 deletions(-)
> > 
> > diff --git a/lib/pdump/rte_pdump.c b/lib/pdump/rte_pdump.c
> > index af450695ec..b3a62df591 100644
> > --- a/lib/pdump/rte_pdump.c
> > +++ b/lib/pdump/rte_pdump.c
> > @@ -2,7 +2,6 @@
> >   * Copyright(c) 2016-2018 Intel Corporation
> >   */
> >  
> > -#include <rte_memcpy.h>
> >  #include <rte_mbuf.h>
> >  #include <rte_ethdev.h>
> >  #include <rte_lcore.h>
> 
> Yes, this code doesn't use rte_memcpy so yes, remove it.
> 
> > 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.

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

/Bruce
  

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

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

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

Using the empty structure definition is reasonable and is done a couple other places.
  

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

> >
> > 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
> 
> Using the empty structure definition is reasonable and is done a couple
> other places.

I still consider bad practice, because compile time weighs infinitely lower than code cleanliness in my cost/benefit analysis.

-Morten
  

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
  

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 3 February 2022 11.43
> 
> 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.

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
  

On Thu, Feb 03, 2022 at 01:11:42PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 3 February 2022 11.43
> > 
> > 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.
> 
> 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. ;-)
>
In this case it probably doesn't matter that much, either way, and thanks
for flagging issues with this!

Hopefully my final word on this is that it can sometimes be more
problematic than just compilation time. I've hit issues in the past when
working on DPDK, where adding in a needed header include to another header,
created an include loop that broke compilation, because one of the headers
in the chain/loop had included another header just to get a reference to a
structure half-way down the file. Ever since, I've had a strong dislike
for including unnecessary headers, especially if it is just for a pointer
to a struct.

Regards,
/Bruce