As a pleasant background task I'm re-programming astime (an X clock) to work more the way I want it to.
It's currently written in ANSI C and shows evidence of different programmers having been involved at different times. Nothing wrong with that but it is a bit inconsistent in the way it does things.
I'm trying to improve its style as well as making it do what I want, so, finally to my questions!
It has *lots* of configuration parameters which specify things like colours of the hands, font to use for text, size of clock, etc. etc. There's something like 50 or so of them.
In the existing code some are globals, some are members of a structure called 'state', there appears to be no good reason for this division.
The problem is how to 'hand them around' between different parts of the code, if they're all globals it's easy to initialise them as well as create them in a file called, say, astime_config.c, this avoids the need to create them and *then* initialise them separately. However, apart from so many globals creating problems of its own, it then requires a 'extern' declaration for every value in a header file so that other source files can access the values.
On the other hand if they're all in a structure (or maybe two or three structures) the structure can be declared in a header file and only a few 'extern' declarations are needed (one for each structure). However initialisation is then messy as it's of the form:-
struct Fred fred = { <lots of numbers and strings> };
where it's difficult to document and keep tabs on all those constants.
Is there a good way around this problem or have I just got to bite the bullet and decide on one or other of the above and go with it?
C++ solutions are just as welcome but I can't see that helping really.
Chris Green chris@areti.co.uk writes:
The problem is how to 'hand them around' between different parts of the code, if they're all globals it's easy to initialise them as well as create them in a file called, say, astime_config.c, this avoids the need to create them and *then* initialise them separately. However, apart from so many globals creating problems of its own, it then requires a 'extern' declaration for every value in a header file so that other source files can access the values.
It's relatively easy to avoid mentioning every global twice. For example:
/* globals.h --------------------------------------------------------------- */
#if DEFINE_GLOBALS # define EXTERN # define INIT(x) =x #else # define EXTERN extern # define INIT(x) #endif
EXTERN int foo INIT(3); EXTERN int bar INIT(99);
/* globals.c --------------------------------------------------------------- */
#define DEFINE_GLOBALS 1 #include "globals.h"
/* any other source file --------------------------------------------------- */
#include "globals.h"
On Thu, Aug 11, 2005 at 10:48:37AM +0100, Richard Kettlewell wrote:
Chris Green chris@areti.co.uk writes:
The problem is how to 'hand them around' between different parts of the code, if they're all globals it's easy to initialise them as well as create them in a file called, say, astime_config.c, this avoids the need to create them and *then* initialise them separately. However, apart from so many globals creating problems of its own, it then requires a 'extern' declaration for every value in a header file so that other source files can access the values.
It's relatively easy to avoid mentioning every global twice. For example:
/* globals.h --------------------------------------------------------------- */
#if DEFINE_GLOBALS # define EXTERN # define INIT(x) =x #else # define EXTERN extern # define INIT(x) #endif
EXTERN int foo INIT(3); EXTERN int bar INIT(99);
/* globals.c --------------------------------------------------------------- */
#define DEFINE_GLOBALS 1 #include "globals.h"
/* any other source file --------------------------------------------------- */
#include "globals.h"
Thanks, yes, I've done similar tricks with #define when wanting a set of enum constants to match a set of string variables. The above certainly does address the globals issue. However it does mean that you have values (other than constants) in the .h file which is (sort of) a bit naughty.
You could of course make all the actual definitions one huge #define and use it in two places with your two sets of EXTERN and INIT defines set differently. This is essentially what I did for my enum/string one.
Thanks for the ideas, I'll probably go down this route or something like it.
Chris Green chris@areti.co.uk writes:
Thanks, yes, I've done similar tricks with #define when wanting a set of enum constants to match a set of string variables. The above certainly does address the globals issue. However it does mean that you have values (other than constants) in the .h file which is (sort of) a bit naughty.
I don't see why...?
On Thu, Aug 11, 2005 at 01:38:20PM +0100, Richard Kettlewell wrote:
Chris Green chris@areti.co.uk writes:
Thanks, yes, I've done similar tricks with #define when wanting a set of enum constants to match a set of string variables. The above certainly does address the globals issue. However it does mean that you have values (other than constants) in the .h file which is (sort of) a bit naughty.
I don't see why...?
.h files are supposed to have no dynamic data in them, otherwise if they're included in different parts of a build you get multiple definitions. Your #define statements making it build in different ways according to where it's called from prevents the multiple definitions but it's rather 'against the philosophy'.
On Thu, Aug 11, 2005 at 12:51:49PM +0100, Chris Green wrote:
You could of course make all the actual definitions one huge #define and use it in two places with your two sets of EXTERN and INIT defines set differently. This is essentially what I did for my enum/string one.
... and going down this route I have come up with:-
#define CLOCK \ {\ VAR (int, xsize, 54)\ VAR (int, ysize, 54)\ VAR (char, bgcolor[100], "#123456")\ VAR (char, fgcolor[100], "#FFFFFF")\ VAR (bool, iconic, false)\ }
#define VAR(a, b, c) a b;
struct Clk CLOCK; // declare the structure
#undef VAR #define VAR(a, b, c) c,
struct Clk clk = CLOCK; // intialiase the structure
int main(int argc, char * argv[]) { int i; }
Using 'CC -E' (this is a Solaris box) shows the output from the pre-processor to be a correctly declared and initialised struct.
Is there a good way around this problem or have I just got to bite the bullet and decide on one or other of the above and go with it?
I am really a C++ developer, but with some C experience.
Really, any coding style should make the code easier to read, and easier to maintain - this includes documentation.
I normally follow these rules for using globals and structures:
Only use globals if absolutely necessary (always try to pass data as arguments to functions, either as values or pointers)
If a variable is required to remain in scope for multiple calls to a function, rather than make it global, make it static in the function that's using it.
Use structures to group related items together. So a structure containing say screen attributes would be OK, but just grouping variables together for the sake of it would not be acceptable.
Also, for documentation, have you tried using Doxygen. I find it does a good job of documentating the code.
Stuart.
On Thu, Aug 11, 2005 at 11:27:27AM +0100, Stuart Bailey wrote:
Is there a good way around this problem or have I just got to bite the bullet and decide on one or other of the above and go with it?
I am really a C++ developer, but with some C experience.
Really, any coding style should make the code easier to read, and easier to maintain - this includes documentation.
I normally follow these rules for using globals and structures:
Only use globals if absolutely necessary (always try to pass data as arguments to functions, either as values or pointers)
Quite agree which is why I wasn't keen on the 'global' solution.
If a variable is required to remain in scope for multiple calls to a function, rather than make it global, make it static in the function that's using it.
Yes, if that's the intention then that's the way to do it.
Use structures to group related items together. So a structure containing say screen attributes would be OK, but just grouping variables together for the sake of it would not be acceptable.
Which is one of the reasons for not having a huge 'config' structure in my original question.
However none of this addresses my question really. The issue is trying to avoid repeating what is essentially the same thing over and over again when you have a large number of loosely associated values (the clock configuration parameters).
If they're globals you have to both initialise them and declare them as extern. Every global is (at least) duplicated as:-
int xsize = 1234; // in a .c file somewhere
and
extern int xsize; // in a .h file somewhere
If (as there should be) there are very few globals this is OK, but with the 50 or more the clock requires it's messy. They *are* required in different places because they're modified by the command line parsing and used in the X code which are separate source files.
If they were passed around as function parameters you'd end up with huge parameter lists to the functions, unless you move to the alternative struct .....
If you use a struct to contain the configuration you avoid many of the nasties of globals, however initialising the values is messy IMHO. You get stuff like:-
In a header file:-
struct ClockConfig { int xsize; int ysize; char bgcolor[COL_SIZE]; char fgcolor[COL_SIZE]; bool iconic; ... ... ... etc. };
and then, in the code somewhere:-
struct ClockConfig clockCfg = { 54, 54, "#123456", "#FFFFFF", FALSE, ... ... .. etc. };
Which, to my mind, is truly horrible and difficult to maintain because you have to be very careful to maintain the one-to-one correspondence between the initialisation values and the struct. Commenting will of course help but it again represents duplicated information which really shouldn't be necessary.
If you use a struct to contain the configuration you avoid many of the nasties of globals, however initialising the values is messy IMHO. You get stuff like:-
In a header file:-
struct ClockConfig { int xsize; int ysize; char bgcolor[COL_SIZE]; char fgcolor[COL_SIZE]; bool iconic; ... ... ... etc. };
and then, in the code somewhere:-
struct ClockConfig clockCfg = { 54, 54, "#123456", "#FFFFFF", FALSE, ... ... .. etc. };
Which, to my mind, is truly horrible and difficult to maintain because you have to be very careful to maintain the one-to-one correspondence between the initialisation values and the struct. Commenting will of course help but it again represents duplicated information which really shouldn't be necessary.
Yes, but as far as I can remember (and I may be verging on C++), you can have functions within a struct. Therefore, you could create an initialisation function, with well named parameters (with defaults), that will setup the struct for you. Although you will still have a long parameter list. Unfortunately, I can't remeber the syntax - and I can't find any of my C books to reference. But basically, a C++ class is essentially a structure whose members are by default private rather than public as in structures.
Stuart
Stuart Bailey stuart@linusoft.co.uk writes:
Yes, but as far as I can remember (and I may be verging on C++), you can have functions within a struct. Therefore, you could create an initialisation function, with well named parameters (with defaults), that will setup the struct for you. Although you will still have a long parameter list. Unfortunately, I can't remeber the syntax - and I can't find any of my C books to reference. But basically, a C++ class is essentially a structure whose members are by default private rather than public as in structures.
You are thinking of a constructor, which is part of C++, not of C. You could indeed use that to tie names to initial values, though it still has the problem that you have to list the names twice, opening up scope for error.
GCC/C99 designated initializer syntax avoids the C++ dependency but has the same flaw.
This email and any files transmitted with it are confidential. If you are not the intended recipient, please email postmaster@linusoft.co.uk immediately. You should not copy or use this email or attachments for any purpose nor disclose their contents to any other person.
NO BINDING CONTRACT WILL RESULT FROM THIS E-MAIL UNTIL SUCH TIME AS A WRITTEN DOCUMENT IS SIGNED ON BEHALF OF LinuSoft.
LinuSoft cannot accept any responsibility for the completeness or accuracy of this message as it has been transmitted over public networks.
This is very silly.
On Thu, Aug 11, 2005 at 01:25:03PM +0100, Stuart Bailey wrote:
If you use a struct to contain the configuration you avoid many of the nasties of globals, however initialising the values is messy IMHO. You get stuff like:-
In a header file:-
struct ClockConfig { int xsize; int ysize; char bgcolor[COL_SIZE]; char fgcolor[COL_SIZE]; bool iconic; ... ... ... etc. };
and then, in the code somewhere:-
struct ClockConfig clockCfg = { 54, 54, "#123456", "#FFFFFF", FALSE, ... ... .. etc. };
Which, to my mind, is truly horrible and difficult to maintain because you have to be very careful to maintain the one-to-one correspondence between the initialisation values and the struct. Commenting will of course help but it again represents duplicated information which really shouldn't be necessary.
Yes, but as far as I can remember (and I may be verging on C++), you can have functions within a struct. Therefore, you could create an initialisation function, with well named parameters (with defaults), that will setup the struct for you.
Does this actually prevent the duplication? You have your struct, with the variables, and you have your function with a parameter list (with defaults) which simply duplicates the variables, same different!
Or have I misunderstood?
Although you will still have a long parameter list. Unfortunately, I can't remeber the syntax - and I can't find any of my C books to reference. But basically, a C++ class is essentially a structure whose members are by default private rather than public as in structures.
I'm quite happy to change to C++ but I can't see how it helps for this particular problem. Any class type approach that I can think of still duplicates everything effectively. Unless you can initialise variables like this:-
class Clock : public { public: int xsize = 54; int ysize = 54; char bgcolor[COL_SIZE] = "#123456"; char fgcolor[COL_SIZE] = "#FFFFFF"; bool iconic = false; }
I know this isn't really what OOP is about (I'm not hiding the data) but if I could do this it would be an excellent way to manage this particular problem.
Coding styles can be an emotive subject (along the lines of which is better, vi or emacs). I for one find the GNU recommended style to be horrible, preferring the guidance found in Documentation/CodingStyle (from any kernel source) to be reasonable.
On Thursday 11 August 2005 13:25, Stuart Bailey wrote:
Yes, but as far as I can remember (and I may be verging on C++), you can have functions within a struct. Therefore, you could create an initialisation function, with well named parameters (with defaults), that will setup the struct for you. Although you will still have a long parameter list. Unfortunately, I can't remeber the syntax - and I can't find any of my C books to reference. But basically, a C++ class is essentially a structure whose members are by default private rather than public as in structures.
struct Foo{ public: Foo() {a=0; b=2; c=3; bar="idunno"};
int a; int b; int c; char bar[80]; }
However, this only works with C++, so one small trick I use is a macro defined in the same header as the struct definition..
typedef struct { double s, x, y, z; } PmRotation;
#define PmRotation_init {1, 0, 0, 0}
In the source, it is a simple matter of doing:
PmRotation foo = PmRotation_init;
Once again, this gets messy if the multitude of structs needed different initialisation values...
Which is better - That really depends on how far you want to go in embracing C++ and OOP. Certainly C++ allows you to hide and protect many variables that would otherwise become global.
Regards, Paul.
On Thu, Aug 11, 2005 at 02:08:58PM +0100, Paul wrote:
Coding styles can be an emotive subject (along the lines of which is better, vi or emacs). I for one find the GNU recommended style to be horrible, preferring the guidance found in Documentation/CodingStyle (from any kernel source) to be reasonable.
On Thursday 11 August 2005 13:25, Stuart Bailey wrote:
Yes, but as far as I can remember (and I may be verging on C++), you can have functions within a struct. Therefore, you could create an initialisation function, with well named parameters (with defaults), that will setup the struct for you. Although you will still have a long parameter list. Unfortunately, I can't remeber the syntax - and I can't find any of my C books to reference. But basically, a C++ class is essentially a structure whose members are by default private rather than public as in structures.
struct Foo{ public: Foo() {a=0; b=2; c=3; bar="idunno"};
int a; int b; int c; char bar[80]; }
However, this only works with C++, so one small trick I use is a macro defined in the same header as the struct definition..
typedef struct { double s, x, y, z; } PmRotation;
#define PmRotation_init {1, 0, 0, 0}
Both these examples show exactly what I'm trying to avoid, the separation into two different lists of the declarations of the variables and their initialisation. When there's a large number of variables it's very difficult to keep track of which initialisation value goes with which variable.
On 11/08/05 13:07:10, Chris Green wrote:
However none of this addresses my question really. The issue is
trying
to avoid repeating what is essentially the same thing over and over again when you have a large number of loosely associated values (the clock configuration parameters).
If they're globals you have to both initialise them and declare them as extern. Every global is (at least) duplicated as:-
int xsize = 1234; // in a .c file somewhere
and
extern int xsize; // in a .h file somewhere
If (as there should be) there are very few globals this is OK, but with the 50 or more the clock requires it's messy. They *are* required in different places because they're modified by the command line parsing and used in the X code which are separate source files.
If you think globals are the best solution then you could adopt a code generation approach.
Create a file called globals.c or something similar and list the variables and their initialisers there, so in your above example:
int xsize = 1234;
Now use a script, perhaps in perl or similar, to process this file by taking each non-comment line, grabbing the part before the '=' sign (or before the final semicolon if there is no '=' on that line), prepend 'extern ' to it and write it out. The output of this script goes into globals.h or similar which can then be included by other modules.
This script can be run from a makefile to to add a new variable you edit globals.c and run make.
Steve.