Re: two xasserts in squid3
On Mon, 2008-02-11 at 12:18 +0900, Adrian Chadd wrote: On Mon, Feb 11, 2008, Amos Jeffries wrote: That only because none has shown me a better way to do function pointers than the way squid currently does them. I'm pretty sure the async calls stuff has an implementation of this. There's other ways - look at asio.sourceforge.net's implementation which uses some Boost methods for creating 0, 1 and 2 argument callbacks of varying types. Do Boost methods for creating callbacks differ substantially from Squid methods? I was under impression they are about the same except Squid ones are less polished and simpler because they cover fewer special cases (that Squid is not using yet). Thank you, Alex.
Re: two xasserts in squid3
On Mon, 2008-02-11 at 10:15 +1300, Amos Jeffries wrote: I think a better approach to this would be: 1) do we actually need it anyway? Yes, we need a common assert-like macro. 2) where is it supposed to be defined? IMO: include/xassert.h if xassert is used outside of src/ src/xassert.h otherwise, until we get something like src/base/ or src/misc for general-purpose commonly used things that do not fit anywhere else. I'm willing to commit time to that if we can agree on the 'old' files that need removing and the scope of the remining ones. I have seen a few things that made me think squid.h and defines.h may need to (merge?) stick around under a better definition of content. I would welcome such cleanup, especially after we switch to a VCS that does file renaming. IMO, there needs to be one header file like squid.h that is guaranteed to be the first one included by all other source files in src/. That file should only contain things needed by 99.9% of the code base. This is usually limited to hacks that change some global system or compiler effects. The rest should have their own purpose-specific header files. If the source code needs something used by others, it needs to include something.h. There is also a question of whether some lib/ stuff needs to be moved to src/, but that is a separate debate and we need a better VCS for that as well. HTH, Alex.
Re: two xasserts in squid3
On Tue, 2008-02-12 at 11:10 +1300, Amos Jeffries wrote: On Mon, 2008-02-11 at 09:33 +1100, Robert Collins wrote: One of the things I'd most love to see is the modularisation completed - complete deletion of protos.h, structs.h, typedefs.h. Yes, of course. That would be the focus of the cleanup that Amos is volunteering to do :-) Hows this for an action plan Robert? * Obsolete typedefs.h (underway) - remove all unneeded typedefs - move all needed typedefs to their appropriate headers - fix compile errors * Add automatic testing for header dependency - script to perform universal include unit-test for .h files - link to automatic unit-testing in each directory - fix the compile errors! * Obsolete protos.h - move all protos to their appropriate header files - add includes for headers where needed. * Obsolete structs.h - move all structs to their appropriate header files - move modular configuration in to *Config.h files (discussion on exactly what the modules are) I like the automatic include unit test. I found while slowly working through this that circular structure dependencies made it quite tricky; I think you will find you need * Break dependencies to make it really clean, but that can be longer term if needed. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: two xasserts in squid3
On Tue, 2008-02-12 at 10:37 +1300, Amos Jeffries wrote:
When we get a better VCS, we should discuss moving include/ and lib/
stuff into src/ with the exception of 3rd party code. This would avoid
problems created by that artificial boundary.
What I have been mulling over after seeing code heirarchy like this:
/include + /lib == C library functions for portability (autotools
requires these here).
In my experience, autotools do not require them to be there or those
requirements can be relaxed as needed. Our portability wrappers may
still use Squid-specific code so placing them outside of src/ is not a
good idea, IMO. There got to be a better reason to place something
outside of src/ than autotools :-).
My reading of the AC_REPLACE_FUNCS(...) is that it when it detects a
portability issue it adds include/missing-function.h and
lib/missing-function.c to the build list.
We are using that for several library functions.
?somewhere? == third party additions (currently /lib/name/*
/lib/name is not too bad. We can even do src/3rdparty/name or something
like that, but perhaps keeping that stuff outside of src/ is a good idea
even though it is also source.
I'm not disagreeing on lib/name/. Just splitting the so its kept seperate
from the raw portability files.
/src/ == core code at lowest level
I would move it to src/base or src/backend or something like that. And
there should not be really many files there.
/src/common ?? (I think you were against a /src/core earlier)
/src/module == 'independant' module code. namely the files like
existing
ICAP, FS, Auth code. But making sure that all the other independant code
gets its own sub-dirs too (thinking each server-side gateway, pinger,
DNS?, ESI, Others?)
Yes, but module and independent should be treated loosely. For
example, logging may not be a module or independent, but may need
its own directory. Same for memory management.
In short, when you have or want to have a bunch of Something*.{cc,h}
files, place them into src/Something/ directory.
Exactly.
Amos
Re: two xasserts in squid3
On Mon, 2008-02-11 at 10:15 +1300, Amos Jeffries wrote: I think a better approach to this would be: 1) do we actually need it anyway? Yes, we need a common assert-like macro. 2) where is it supposed to be defined? IMO: include/xassert.h if xassert is used outside of src/ src/xassert.h otherwise, until we get something like src/base/ or src/misc for general-purpose commonly used things that do not fit anywhere else. I'm willing to commit time to that if we can agree on the 'old' files that need removing and the scope of the remining ones. I have seen a few things that made me think squid.h and defines.h may need to (merge?) stick around under a better definition of content. I would welcome such cleanup, especially after we switch to a VCS that does file renaming. IMO, there needs to be one header file like squid.h that is guaranteed to be the first one included by all other source files in src/. That file should only contain things needed by 99.9% of the code base. This is usually limited to hacks that change some global system or compiler effects. The rest should have their own purpose-specific header files. I was thinking that squid.h is the good choice for that. Just with much less tangled dependancy than it currently has. With some that is currently in defines.h. FWIW: I have a test run going over typedefs and managed to kill a few dozen lines of code there without side-effects on linux. I'll have it in cleanup branch soon for testing on other OS and compilers. If the source code needs something used by others, it needs to include something.h. There is also a question of whether some lib/ stuff needs to be moved to src/, but that is a separate debate and we need a better VCS for that as well. Some does, ie IPAddress I think should go back. My initial reason for placing it there is no longer valid. IMO Mem* and Debug shouls not be there either, but in wherever the core goes. Amos
Re: two xasserts in squid3
On Sun, 2008-02-10 at 20:51 +0200, Tsantilas Christos wrote: Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... It looks like Array, MemPool, and splay files are using assert outside of src/ so you will not be able to remove include/assert.h. However, you may be able to declare xassert in include/assert.h while defining it in src/Debug.cc. The assert #define in src/Debug.h should probably be moved to include/assert.h. These #defines must be the same. When we get a better VCS, we should discuss moving include/ and lib/ stuff into src/ with the exception of 3rd party code. This would avoid problems created by that artificial boundary. What I have been mulling over after seeing code heirarchy like this: /include + /lib == C library functions for portability (autotools requires these here). ?somewhere? == third party additions (currently /lib/name/* /src/ == core code at lowest level /src/module == 'independant' module code. namely the files like existing ICAP, FS, Auth code. But making sure that all the other independant code gets its own sub-dirs too (thinking each server-side gateway, pinger, DNS?, ESI, Others?) Have I understood the earlier plans right? I think its a good structure, but as you say needs a better VCS to implement fully. Amos
Re: two xasserts in squid3
Alex Rousskov wrote: On Sun, 2008-02-10 at 20:51 +0200, Tsantilas Christos wrote: Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... It looks like Array, MemPool, and splay files are using assert outside of src/ so you will not be able to remove include/assert.h. However, you may be able to declare xassert in include/assert.h while defining it in src/Debug.cc. It is not the best to have the declaration at library headers (include/*) and the definition in program sources, but yes looks that it is the safest solution at this time. Should I fix it in async-calls branch, or someone else will do the job in squid3-HEAD? I have yesterday created a branch cleanup in Squid-3 for the purpose of performing the larger cleanup operations instead of HEAD. This is so we can maintain better stability in HEAD and the bigger or more experimental changes can be done and tested in cleanup before going mainstream. Feel free to do it there and see what breaks. Amos The assert #define in src/Debug.h should probably be moved to include/assert.h. These #defines must be the same. Are not exactly the same. In the Debug.h implementation there is an extra #if PURIFY . The assert.h file included at the begining of squid.h file and the Debug.h included in defines.h file which also included at the squid.h. It is not clear with a first view which assert called, and I wondering if there is a trick here..
Re: two xasserts in squid3
Alex Rousskov wrote: On Sun, 2008-02-10 at 20:51 +0200, Tsantilas Christos wrote: Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... It looks like Array, MemPool, and splay files are using assert outside of src/ so you will not be able to remove include/assert.h. However, you may be able to declare xassert in include/assert.h while defining it in src/Debug.cc. It is not the best to have the declaration at library headers (include/*) and the definition in program sources, but yes looks that it is the safest solution at this time. Should I fix it in async-calls branch, or someone else will do the job in squid3-HEAD? The assert #define in src/Debug.h should probably be moved to include/assert.h. These #defines must be the same. Are not exactly the same. In the Debug.h implementation there is an extra #if PURIFY . The assert.h file included at the begining of squid.h file and the Debug.h included in defines.h file which also included at the squid.h. It is not clear with a first view which assert called, and I wondering if there is a trick here..
Re: two xasserts in squid3
On Sun, 2008-02-10 at 20:51 +0200, Tsantilas Christos wrote: Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... It looks like Array, MemPool, and splay files are using assert outside of src/ so you will not be able to remove include/assert.h. However, you may be able to declare xassert in include/assert.h while defining it in src/Debug.cc. The assert #define in src/Debug.h should probably be moved to include/assert.h. These #defines must be the same. When we get a better VCS, we should discuss moving include/ and lib/ stuff into src/ with the exception of 3rd party code. This would avoid problems created by that artificial boundary. Alex.
Re: two xasserts in squid3
On Mon, 2008-02-11 at 09:33 +1100, Robert Collins wrote: One of the things I'd most love to see is the modularisation completed - complete deletion of protos.h, structs.h, typedefs.h. Yes, of course. That would be the focus of the cleanup that Amos is volunteering to do :-) Alex.
Re: two xasserts in squid3
On Tue, 2008-02-12 at 11:27 +1300, Amos Jeffries wrote: * Add automatic testing for header dependency - script to perform universal include unit-test for .h files - link to automatic unit-testing in each directory - fix the compile errors! I am thinking along the lines of unit-testing the compilation of each *.h file. At present a fvery few of the important .h files have a hard-coded .cc in the tests directory and that gets built when unit-testing. All it does is #include something.h What I'm planning is a test-suite script that when passed a directory, scans it for all .h files. Writes the .cc and attempts a build. Any compile errors are dependancy errors needing a fix. That would speed-track the dependancy testing in future with no need for manually adding these tests, or a large set of files for them. It is particularly useful for the struct and proto re-arranging. Sounds good. Thank you, Alex.
Re: two xasserts in squid3
On Tue, 2008-02-12 at 11:10 +1300, Amos Jeffries wrote: On Mon, 2008-02-11 at 09:33 +1100, Robert Collins wrote: One of the things I'd most love to see is the modularisation completed - complete deletion of protos.h, structs.h, typedefs.h. Yes, of course. That would be the focus of the cleanup that Amos is volunteering to do :-) Hows this for an action plan Robert? * Obsolete typedefs.h (underway) - remove all unneeded typedefs - move all needed typedefs to their appropriate headers - fix compile errors * Add automatic testing for header dependency - script to perform universal include unit-test for .h files - link to automatic unit-testing in each directory - fix the compile errors! * Obsolete protos.h - move all protos to their appropriate header files - add includes for headers where needed. * Obsolete structs.h - move all structs to their appropriate header files - move modular configuration in to *Config.h files (discussion on exactly what the modules are) I like the automatic include unit test. I found while slowly working through this that circular structure dependencies made it quite tricky; Yes found that already, and have not even started. ;-) I can't believe I'm thankful that many of them use pointer-fields. I think you will find you need * Break dependencies Please explain. I assume you mean the class Something; pre-definitions instead of #includes. to make it really clean, but that can be longer term if needed. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt.
Re: two xasserts in squid3
Hi all, There is something which confuse me. The xassert function implemented in squid3 in two places, in the file src/debug.cc and in the lib/assert.c file. Also the assert macro declared in src/Debug.h file and in the include/assert.h file. Is there any reason for these two implementations of xassert function? Maybe it was better if the files lib/assert.c and include/assert.h removed and the assert macro defined in squid.h file like squid2.6 does... I think a better approach to this would be: 1) do we actually need it anyway? 2) where is it supposed to be defined? What I'm seeing with the auto-docs work is that a lot of header files include squid.h or protos.h for one or two simple things. That file brings with it a host of type-dependencies, directly or indirectly that clutter up the whole header include process for compiling. I agree with keeping some of the global macros defined in squid.h or defines.h (ie SQUID_MAXHOSTNAMELEN and other boundary squid-wide definitions), but want to see needless dependency removed in the long-term plan. That would involved splitting some of them (like this) out into these assert.h files to be included into places like Debug.h, instead of re-defined there, or adding a dependency of Debug.h on a generic squid.h. _Particularly_ removing all items from the generic squid.h/globals.h/defines.h that add a squid-defined type of any sort. (I know I've been guilty of adding IPAddress there too) If a macro usage is limited to a private .cc file. it would be more appropriate to define it just for that file's compilation and best it go there directly. /rant for the squid-3 plans. 3) can we do any better? Forward-looking, are there any problems with it while its being reviewed that can be fixed now? Amos
Re: two xasserts in squid3
On Mon, 2008-02-11 at 10:15 +1300, Amos Jeffries wrote: What I'm seeing with the auto-docs work is that a lot of header files include squid.h or protos.h for one or two simple things. That file brings with it a host of type-dependencies, directly or indirectly that clutter up the whole header include process for compiling. One of the things I'd most love to see is the modularisation completed - complete deletion of protos.h, structs.h, typedefs.h. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: two xasserts in squid3
On Mon, 2008-02-11 at 10:15 +1300, Amos Jeffries wrote: What I'm seeing with the auto-docs work is that a lot of header files include squid.h or protos.h for one or two simple things. That file brings with it a host of type-dependencies, directly or indirectly that clutter up the whole header include process for compiling. One of the things I'd most love to see is the modularisation completed - complete deletion of protos.h, structs.h, typedefs.h. Ditto for globals.h. (Every global has a type yes? thus a unique header where its appropriate.) I'm willing to commit time to that if we can agree on the 'old' files that need removing and the scope of the remining ones. I have seen a few things that made me think squid.h and defines.h may need to (merge?) stick around under a better definition of content. FWIW the keyword 'typedef' is as evil in C++ as #define, and only 'goto' is worse (and completely unnecessary since functional-C deprecated pascal). yes folks, we have all three in Squid still! Amos
Re: two xasserts in squid3
On Mon, Feb 11, 2008, Amos Jeffries wrote: FWIW the keyword 'typedef' is as evil in C++ as #define, and only 'goto' is worse (and completely unnecessary since functional-C deprecated pascal). Goto is fine. You just need to know how and when to use it. My use of goto in new Squid C code I write is purely to unwind whatever stuff has been allocated/locked in the function before I exit. That way I end up with -one- exit location in the function and I don't have to bum around using extra functions or massively nested conditional constructs to achieve it. In fact, I've used goto in a few places to tidy up the code.. Adrian -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: two xasserts in squid3
On Mon, Feb 11, 2008, Amos Jeffries wrote: FWIW the keyword 'typedef' is as evil in C++ as #define, and only 'goto' is worse (and completely unnecessary since functional-C deprecated pascal). Goto is fine. You just need to know how and when to use it. My use of goto in new Squid C code I write is purely to unwind whatever stuff has been allocated/locked in the function before I exit. That way I end up with -one- exit location in the function and I don't have to bum around using extra functions or massively nested conditional constructs to achieve it. In fact, I've used goto in a few places to tidy up the code.. If you need to use it to clean up a single function. It's an obvious sign that the function is too complicated. In 10 years of writing complicated code I have yet to see a single place where it is actually required. Amos
Re: two xasserts in squid3
On Mon, Feb 11, 2008, Amos Jeffries wrote: I end up with -one- exit location in the function and I don't have to bum around using extra functions or massively nested conditional constructs to achieve it. In fact, I've used goto in a few places to tidy up the code.. If you need to use it to clean up a single function. It's an obvious sign that the function is too complicated. In 10 years of writing complicated code I have yet to see a single place where it is actually required. Required? No. Useful? Yes. Too often I've seen people push the cleanup stuff into another function, thinking that bit of refactoring was a good idea. Thing is, if they didn't document that the function is private just for another function, some other coder will come along later, see the function, and think its fine for them to use (solving their immediate problem.) This leads to badness in the future. It took me quite a while to get over the goto is evil, never ever use it koolaid. But then, in C++, you should be using exceptions, not weird flow control tricks. :) Adrian -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: two xasserts in squid3
On Mon, Feb 11, 2008, Amos Jeffries wrote: I end up with -one- exit location in the function and I don't have to bum around using extra functions or massively nested conditional constructs to achieve it. In fact, I've used goto in a few places to tidy up the code.. If you need to use it to clean up a single function. It's an obvious sign that the function is too complicated. In 10 years of writing complicated code I have yet to see a single place where it is actually required. Required? No. Useful? Yes. Too often I've seen people push the cleanup stuff into another function, thinking that bit of refactoring was a good idea. Thing is, if they didn't document that the function is private just for another function, some other coder will come along later, see the function, and think its fine for them to use (solving their immediate problem.) This leads to badness in the future. It took me quite a while to get over the goto is evil, never ever use it koolaid. But then, in C++, you should be using exceptions, not weird flow control tricks. :) I've never fully subscribed to that generalisation. But the arguments made in its favour have made me think about the necessity of several constructs use in code. In general I have found goto is largely useless. Where its most tempting is in badly designed code. typedef has its place in C particularly for portability issues, but that is vastly reduced in C++. I've only seen the event/callback function-pointers as a required use for it nowadays. That only because none has shown me a better way to do function pointers than the way squid currently does them. Amos
Re: two xasserts in squid3
On Mon, Feb 11, 2008, Amos Jeffries wrote: It took me quite a while to get over the goto is evil, never ever use it koolaid. But then, in C++, you should be using exceptions, not weird flow control tricks. :) I've never fully subscribed to that generalisation. But the arguments made in its favour have made me think about the necessity of several constructs use in code. In general I have found goto is largely useless. Where its most tempting is in badly designed code. typedef has its place in C particularly for portability issues, but that is vastly reduced in C++. I've only seen the event/callback function-pointers as a required use for it nowadays. That only because none has shown me a better way to do function pointers than the way squid currently does them. I'm pretty sure the async calls stuff has an implementation of this. There's other ways - look at asio.sourceforge.net's implementation which uses some Boost methods for creating 0, 1 and 2 argument callbacks of varying types. Adrian
Re: two xasserts in squid3
On Mon, 2008-02-11 at 11:49 +0900, Adrian Chadd wrote: On Mon, Feb 11, 2008, Amos Jeffries wrote: I end up with -one- exit location in the function and I don't have to bum around using extra functions or massively nested conditional constructs to achieve it. In fact, I've used goto in a few places to tidy up the code.. If you need to use it to clean up a single function. It's an obvious sign that the function is too complicated. In 10 years of writing complicated code I have yet to see a single place where it is actually required. Required? No. Useful? Yes. Too often I've seen people push the cleanup stuff into another function, thinking that bit of refactoring was a good idea. Thing is, if they didn't document that the function is private just for another function, some other coder will come along later, see the function, and think its fine for them to use (solving their immediate problem.) This leads to badness in the future. It took me quite a while to get over the goto is evil, never ever use it koolaid. But then, in C++, you should be using exceptions, not weird flow control tricks. :) I quite like the very structured way of using goto: that the linux kernel encourages. Its certainly cleaner the way they do it than reused helper functions - you need a bazjillion little functions to clean up nested resources properly, and you end up with two sets: a tonne of little 'call X cleanup, then chain to Y the remaining things to cleanup' functions. And secondly, all the actual X cleanup functions. -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: two xasserts in squid3
On Mon, 2008-02-11 at 16:03 +1300, Amos Jeffries wrote: typedef has its place in C particularly for portability issues, but that is vastly reduced in C++. I've only seen the event/callback function-pointers as a required use for it nowadays. That only because none has shown me a better way to do function pointers than the way squid currently does them. I've seen a lot of tasteful typedefs in c++ to provide handles to common template usage. - in the stdlib IIRC :). -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
