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