Re: [PATCH] proc_fs.h redux
Em Sun, Oct 28, 2007 at 09:44:41AM +0100, Sam Ravnborg escreveu: > > > > As a general rule, I think it better to use includes > > than use naked forward declarations. > > Quite the opposite - at least in the kernel source. > The general rule is that a .h file shall include the > .h files which contain declarations used by said .h files. > But naked declarations as above is preferred over including > a full header file. yup > We see the full header dependency thing to blow off when > inline function are used - which is more and more the case. > In several cases we have converted inline functions to macros > just to simplify the nightmare of header dependencies we have. > > Arnaldo have a nice script that generate a .ps file > showing all the dependencies. > He lately posted this URL: http://oops.ghostprotocols.net:81/acme/tcp.h.ps Well, not "lately", it has been quite a while. But lets celebrate the fact that there is somebody trying to fight this battle one more time and update this tcp.h dependency tree... /me looks for hviz... and if graphviz is installed, ok: http://www.kernel.org/pub/linux/kernel/people/acme/hviz hviz include/linux/tcp.h 10 | dot -Tpdf > /tmp/tcp.h.2007_11.pdf http://oops.ghostprotocols.net:81/acme/tcp.h.2007_11.pdf We still get to sched.h, but before it was: linux/tcp.h -> linux/skbuff.h -> linux/mm.h -> linux/sched.h Nowadays its: linux/tcp.h -> linux/sock.h -> linux/netdevice.h -> linux/interrupt.h -> linux/sched.h And I just removed "#include from linux/interrupt.h, because as far as I checked it is completely unnecessary, and the kernel builds just fine :-) - Arnaldo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
Hi, On Sunday 28 October 2007, Russell King wrote: > On Sat, Oct 27, 2007 at 03:40:04PM -0700, Joe Perches wrote: > > and forward declarations of > > > > struct proc_dir_entry; > > struct file_operations; > > > > As a general rule, I think it better to use includes > > than use naked forward declarations. > > If you go down that route, you end up with _lots_ of circular > dependencies - header file X needs Y needs Z which needs X. We've > been there, several times. It very quickly becomes quite > unmaintainable - you end up with hard to predict behaviour from > include files. > > The only realistic solution is to use forward declarations. It's unfortunately more complicated than that. What basically needs to be done is to separate declarations from its users (usually inline functions). The problem is to correctly pull the declarations from the and header in the correct order. A typical mistake is to put declarations and inline functions in the same linux header and then include some additional from an asm header. For most high level header it's not much of a problem, but let's take as example. struct list_head is used everywhere, but just to get this one definition one also gets quite a few other dependencies as well. bye, Roman - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
* Russell King <[EMAIL PROTECTED]> [2007-10-28 14:04]: > On Sun, Oct 28, 2007 at 12:59:52PM +0100, Bernhard Walle wrote: > > * Russell King <[EMAIL PROTECTED]> [2007-10-28 11:34]: > > > > > > If you go down that route, you end up with _lots_ of circular > > > dependencies - header file X needs Y needs Z which needs X. We've > > > been there, several times. It very quickly becomes quite > > > unmaintainable - you end up with hard to predict behaviour from > > > include files. > > > > > > The only realistic solution is to use forward declarations. > > > > In header files, yes. But that's not true for implementation files. > > I don't think that needs saying - it's quite obvious. You can't > access the contents of structures without their definitions being > available. Of course. But there might be the case where an implementation file doesn't access the structure itself but just passes the pointer to some other function (which is implemented in another file). In that case, you also have the choice between forward declaration and including the header file in the implementation file. Thanks, Bernhard - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
On Sun, Oct 28, 2007 at 12:59:52PM +0100, Bernhard Walle wrote: > * Russell King <[EMAIL PROTECTED]> [2007-10-28 11:34]: > > > > If you go down that route, you end up with _lots_ of circular > > dependencies - header file X needs Y needs Z which needs X. We've > > been there, several times. It very quickly becomes quite > > unmaintainable - you end up with hard to predict behaviour from > > include files. > > > > The only realistic solution is to use forward declarations. > > In header files, yes. But that's not true for implementation files. I don't think that needs saying - it's quite obvious. You can't access the contents of structures without their definitions being available. -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
* Russell King <[EMAIL PROTECTED]> [2007-10-28 11:34]: > > If you go down that route, you end up with _lots_ of circular > dependencies - header file X needs Y needs Z which needs X. We've > been there, several times. It very quickly becomes quite > unmaintainable - you end up with hard to predict behaviour from > include files. > > The only realistic solution is to use forward declarations. In header files, yes. But that's not true for implementation files. Thanks, Bernhard - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
On Oct 28 2007 10:34, Russell King wrote: >On Sat, Oct 27, 2007 at 03:40:04PM -0700, Joe Perches wrote: >> and forward declarations of >> >> struct proc_dir_entry; >> struct file_operations; >> >> As a general rule, I think it better to use includes >> than use naked forward declarations. > >If you go down that route, you end up with _lots_ of circular >dependencies - header file X needs Y needs Z which needs X. We've >been there, several times. It very quickly becomes quite >unmaintainable - you end up with hard to predict behaviour from >include files. > >The only realistic solution is to use forward declarations. Especially because it reduces the amount of I/O that needs to be done. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
On Sat, Oct 27, 2007 at 03:40:04PM -0700, Joe Perches wrote: > and forward declarations of > > struct proc_dir_entry; > struct file_operations; > > As a general rule, I think it better to use includes > than use naked forward declarations. If you go down that route, you end up with _lots_ of circular dependencies - header file X needs Y needs Z which needs X. We've been there, several times. It very quickly becomes quite unmaintainable - you end up with hard to predict behaviour from include files. The only realistic solution is to use forward declarations. -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
> > As a general rule, I think it better to use includes > than use naked forward declarations. Quite the opposite - at least in the kernel source. The general rule is that a .h file shall include the .h files which contain declarations used by said .h files. But naked declarations as above is preferred over including a full header file. We see the full header dependency thing to blow off when inline function are used - which is more and more the case. In several cases we have converted inline functions to macros just to simplify the nightmare of header dependencies we have. Arnaldo have a nice script that generate a .ps file showing all the dependencies. He lately posted this URL: http://oops.ghostprotocols.net:81/acme/tcp.h.ps Sam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
On Sat, Oct 27, 2007 at 03:40:04PM -0700, Joe Perches wrote: > On Sat, 2007-10-27 at 23:47 +0400, Alexey Dobriyan wrote: > > Remove proc_fs.h from headers that doesn't really need it. > > > --- a/arch/powerpc/kernel/process.c > > +++ b/arch/powerpc/kernel/process.c > > @@ -17,6 +17,7 @@ > > #include > > #include > > #include > > +#include > > Your code doesn't match your patch description. > > You've got new includes of: > > > > > > > and forward declarations of > > struct proc_dir_entry; > struct file_operations; Removal of header A from header B creates compilation breakages for files which were getting A indirectly. So you have to complensate in all such cases. > As a general rule, I think it better to use includes > than use naked forward declarations. Well, we also want faster compilation and less time wasted when test-compiling patches. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc_fs.h redux
On Sat, 2007-10-27 at 23:47 +0400, Alexey Dobriyan wrote: > Remove proc_fs.h from headers that doesn't really need it. > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include Your code doesn't match your patch description. You've got new includes of: and forward declarations of struct proc_dir_entry; struct file_operations; As a general rule, I think it better to use includes than use naked forward declarations. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] proc_fs.h redux
Remove proc_fs.h from headers that doesn't really need it. Typical overkill is including full header when one can get away with just forward declaration of "struct proc_dir_entry". Number of files that are recompiled after touching proc_fs.h drops from 1100 to 513(!) on x86_64 allmodconfig. Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]> --- arch/powerpc/kernel/process.c |1 + arch/powerpc/kernel/prom.c |1 + arch/powerpc/mm/init_32.c |1 + arch/powerpc/mm/init_64.c |1 + arch/powerpc/platforms/iseries/mf.c |1 + arch/powerpc/platforms/pasemi/electra_ide.c |2 +- arch/powerpc/sysdev/fsl_soc.c |1 + arch/powerpc/sysdev/mv64x60_dev.c |2 +- arch/powerpc/sysdev/qe_lib/qe.c |1 + arch/sparc/kernel/process.c |3 ++- arch/sparc64/kernel/ldc.c |1 + arch/sparc64/kernel/mdesc.c |1 + arch/sparc64/kernel/viohs.c |2 +- drivers/acpi/bay.c |1 + drivers/acpi/dock.c |2 +- drivers/block/cpqarray.h|2 +- drivers/hwmon/ibmpex.c |2 +- drivers/isdn/hardware/eicon/platform.h |1 - drivers/md/md.c |1 + drivers/misc/fujitsu-laptop.c |1 + drivers/misc/msi-laptop.c |1 + drivers/misc/thinkpad_acpi.c|2 +- drivers/misc/thinkpad_acpi.h|3 ++- drivers/net/bonding/bonding.h |3 ++- drivers/net/cxgb3/t3cdev.h |2 +- drivers/parport/parport_serial.c|2 +- drivers/sbus/char/envctrl.c |2 +- drivers/sbus/char/uctrl.c |1 + drivers/scsi/imm.h |1 - drivers/scsi/ppa.h |1 - drivers/usb/gadget/lh7a40x_udc.h|1 - fs/nfs/client.c |1 + include/acpi/acpi_bus.h |2 +- include/asm-powerpc/prom.h |3 ++- include/asm-sparc/prom.h|4 +++- include/asm-sparc64/prom.h |4 +++- include/linux/atmdev.h |2 +- include/linux/crash_dump.h |3 ++- include/linux/efi.h |1 - include/linux/ipmi.h|1 - include/linux/netfilter.h |2 +- include/linux/parport.h |1 - include/linux/raid/md.h |1 - include/linux/sunrpc/cache.h|4 +++- include/linux/sunrpc/rpc_pipe_fs.h |2 ++ include/linux/sunrpc/stats.h|3 ++- include/linux/wanrouter.h |3 ++- include/net/sctp/sctp.h |2 +- net/atm/br2684.c|1 + net/atm/resources.h |2 -- net/irda/irnet/irnet.h |1 - net/irda/irnet/irnet_irda.c |2 +- net/sctp/objcnt.c |1 + net/sctp/proc.c |1 + net/sctp/protocol.c |1 + net/sunrpc/auth.c |2 ++ net/sunrpc/auth_unix.c |2 +- net/sunrpc/rpcb_clnt.c |1 + net/sunrpc/xprt.c |2 +- net/wanrouter/wanmain.c |1 + net/wanrouter/wanproc.c |1 + 61 files changed, 66 insertions(+), 38 deletions(-) --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include --- a/arch/powerpc/mm/init_32.c +++ b/arch/powerpc/mm/init_32.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include --- a/arch/powerpc/mm/init_64.c +++ b/arch/powerpc/mm/init_64.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include --- a/arch/powerpc/platforms/iseries/mf.c +++ b/arch/powerpc/platforms/iseries/mf.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include --- a/arch/powerpc/platforms/pasemi/electra_ide.c +++ b/arch/powerpc/platforms/pasemi/electra_ide.c @@ -16,7 +16,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ - +#include #include #include --- a/arch/powerpc/sysdev/fsl_soc.c +++ b/arch/powerpc/sysdev/fsl_soc.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #i