> From: Stephen Hemminger [mailto:step...@networkplumber.org] > Sent: Wednesday, 2 February 2022 18.29 > > 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.
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
