Re: two xasserts in squid3

[Alex Rousskov]

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 "
> 
> 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

[Amos Jeffries]

>
> 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: .
>

Re: two xasserts in squid3

[Robert Collins]

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: .


signature.asc
Description: This is a digitally signed message part

Re: two xasserts in squid3

[Amos Jeffries]

>
> On Tue, 2008-02-12 at 11:10 +1300, Amos Jeffries wrote:
>
>> * 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)
>
>> * Auto-doc the API for modules decided above
>>
>> * Move files into appropriate sub-dirs based on modules
>
> Amos,
>
> All sounds good, except I do not understand the "Add automatic
> testing for header dependency" blob. Can you describe that in more
> detail? What is it, and why do we need it?

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 "

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.

Amos

Re: two xasserts in squid3

[Amos Jeffries]

> 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//*
>
> /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/ == '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

[Alex Rousskov]

On Tue, 2008-02-12 at 11:10 +1300, Amos Jeffries wrote:

> * 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)

> * Auto-doc the API for modules decided above
> 
> * Move files into appropriate sub-dirs based on modules

Amos,

All sounds good, except I do not understand the "Add automatic
testing for header dependency" blob. Can you describe that in more
detail? What is it, and why do we need it?

Thank you,

Alex.

Re: two xasserts in squid3

[Amos Jeffries]

> 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)


And the long term end...

* Auto-doc the API for modules decided above

* Move files into appropriate sub-dirs based on modules


Amos

Re: two xasserts in squid3

[Alex Rousskov]

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" :-).

> ?somewhere? == third party additions (currently /lib//*

/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".

> /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/ == '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.

Alex.

Re: two xasserts in squid3

[Amos Jeffries]

> 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

[Amos Jeffries]

>
> 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//*

/src/ == core code at lowest level

/src/ == '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

[Amos Jeffries]

> 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

[Tsantilas Christos]

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

[Alex Rousskov]

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

[Alex Rousskov]

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

[Alex Rousskov]

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

[Alex Rousskov]

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

[Robert Collins]

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: .


signature.asc
Description: This is a digitally signed message part

Re: two xasserts in squid3

[Robert Collins]

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: .


signature.asc
Description: This is a digitally signed message part

Re: two xasserts in squid3

[Adrian Chadd]

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

[Amos Jeffries]

> 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

[Adrian Chadd]

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

[Amos Jeffries]

> 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

[Adrian Chadd]

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

[Amos Jeffries]

>
> 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

[Robert Collins]

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: .


signature.asc
Description: This is a digitally signed message part

Re: two xasserts in squid3

[Amos Jeffries]

> 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