On Thu, Feb 03, 2022 at 01:11:42PM +0100, Morten Brørup wrote: > > 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. > ;-) > 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
