> From: Bruce Richardson [mailto:bruce.richard...@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: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. > > > > 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
