Request for source code review

[Jan Kuiken]

On 8/29/12 21:19 , Udyant Wig wrote:

....

Being a novice, I request the readers of this newsgroup to look over the
source to suggest improvements regarding readability, layout, style,
correctness, idioms, etc. I would appreciate it very much.

Very readable code, good code comments.

One point though, AFAIK the keywords 'extern' in the header files are
not necessary.

Jan Kuiken

 

[Keith Thompson]

#define NDICE 5 /* The number of dice to be rolled. */
#define SIDES 6 /* Each die has these many sides. */
#define MAXSUM (SIDES * NDICE) /* The maximum possible sum of NDICE
dice.
*/
#define LIMIT ((MAXSUM) + 1) /* This constant is used:
0) to declare the array of sums
1) as a loop limit
*/

[...]

The inner parentheses in the definition of LIMIT are unnecessary.
As long as MAXSUM is defined properly (which it is), you can
just write:

#define LIMIT (MAXSUM + 1)

In general, you don't need to parenthesize single tokens
(identifiers, constants) in macro definitions.

 

[Les Cargill]

I've had hours of fun with clearmake...

Sun's (d)make uses the output of the preprocessor phase of compiles to
collate dependency information. Rather more elegant and a lot cheaper!

*nix "make" makefiles can easily say :
depend:
$(CC) -M $(CFLAGS) *.c > .depend
....

include .depend

 

[Adrian Ratnapala]

A decent make should provide a means for automating this. Otherwise
where do you stop recursing up the include paths?

And when I use an *indecent* make, I actually don't worry about this.
Whenver I do an "important" build ("make dist", or whatever) then
automated tools do a complete rebuild.

For the tight edit-test-fix cycle, I just try to keep the dependencies
in my head, and do a "make clean" whenever I see a problem. Of course
I often get it wrong, but I do a "make clean" often enough that it
doesn't get out of hand.

 

[Adrian Ratnapala]

The inner parentheses in the definition of LIMIT are unnecessary.

As long as MAXSUM is defined properly (which it is), you can
just write:

#define LIMIT (MAXSUM + 1)

In general, you don't need to parenthesize single tokens
(identifiers, constants) in macro definitions.

I suspect the reason the original poster put MAXSUM in parentheses is
because he had read that macro *arguments* need to be a protected.
And that is correct!

You *do* need to parethesize arguments. Thus you need to do

#define MULTIPLY_THE_HARD_WAY(A, B) ( (A) * (B) )

because otherwise MULTIPLY_THE_HARD_WAY(x+y, 4) would expand to (x +
y*4); which would be wrong.

 

[Keith Thompson]

I suspect the reason the original poster put MAXSUM in parentheses is
because he had read that macro *arguments* need to be a protected.
And that is correct!

You *do* need to parethesize arguments. Thus you need to do

#define MULTIPLY_THE_HARD_WAY(A, B) ( (A) * (B) )

because otherwise MULTIPLY_THE_HARD_WAY(x+y, 4) would expand to (x +
y*4); which would be wrong.

You're right, of course. Single tokens in macro definitions don't
generally need to be parenthesized *unless* they're macro arguments.
Silly of me to miss that.

 

[James Kuyper]

*nix "make" makefiles can easily say :
depend:
$(CC) -M $(CFLAGS) *.c > .depend
...

include .depend

I tried that for several years. It ended up being more trouble than it
was worth, partly because .depend was always one version out of date
compared to the current code. Unfortunately, I don't remember the details.

What clearmake did was much more comprehensive - it tracked down all
kinds of file dependencies, not just header files, (which can be helpful
when the build script included commands other than those that invoke a
compiler) and the user never had to deal with how or where that
information was stored. As I remember, it "just worked" (at least, the
dependency tracking did - other parts of the system were a little more
complicated to deal with).

 

[Les Cargill]

I tried that for several years. It ended up being more trouble than it
was worth, partly because .depend was always one version out of date
compared to the current code. Unfortunately, I don't remember the details.

You only need to "make depend" when a file has its includes changed. And
in a pinch, you can always utter "gcc -M sourcefile.c >> .depend".

What clearmake did was much more comprehensive - it tracked down all
kinds of file dependencies, not just header files, (which can be helpful
when the build script included commands other than those that invoke a
compiler) and the user never had to deal with how or where that
information was stored.

I would have to use it for months before I could trust it.

As I remember, it "just worked" (at least, the
dependency tracking did - other parts of the system were a little more
complicated to deal with).

Never used clearmake; only clearcase. Was less than impressed with
clearcase.

 

[Ian Collins]

You only need to "make depend" when a file has its includes changed. And
in a pinch, you can always utter "gcc -M sourcefile.c>> .depend".

Sticking one instance of .KEEP_STATE: in your makefile is way more elegant.

 

[Les Cargill]

Sticking one instance of .KEEP_STATE: in your makefile is way more elegant.

Neat! I'd agree.

That appears to be specific to Sun make.

 

[Udyant Wig]

| Very readable code, good code comments.
Thank you.

| One point though, AFAIK the keywords 'extern' in the header files are
| not necessary.
I was following Steve Summit's advice found in section 5.2
"Function Prototypes" of his notes here:
http://www.eskimo.com/~scs/cclass/notes/sx5b.html

He mentions that extern is optional in function declarations, but
that getting into the habit of writing it for all external declarations
is an easier rule to remember.

| Jan Kuiken

 

[Jan Kuiken]

| One point though, AFAIK the keywords 'extern' in the header files are
| not necessary.

I was following Steve Summit's advice found in section 5.2
"Function Prototypes" of his notes here:
http://www.eskimo.com/~scs/cclass/notes/sx5b.html

He mentions that extern is optional in function declarations, but
that getting into the habit of writing it for all external declarations
is an easier rule to remember.

OK, I prefer to use 'extern' only when it is really necessary, as a
'heads up', something funny is going on.

Jan Kuiken

 

[Seungbeom Kim]

* Finally, I prefer to avoid having extra variables. Rather than:

int sum; /* Sum of the dice. */
int i;

for (i = 0; i < maxrolls; i++) {
sum = sumdice (NDICE);
++sums [sum];
}

I'd write:

for (i = 0; i < maxrolls; i++)
++sums[ sumdice(NDICE) ];

On the other hand, some people prefer that one statement should
do only one thing, and that also makes debugging easier: you can
insert a printf to examine the return value of sumdice, and in a
debugger you can pause the execution after calling sumdice but before
incrementing sums (with a breakpoint or by line-by-line execution).

I'd just limit the scope of the variable to the loop:

int i; /* in case you're stuck to C90 */

for (i = 0; i < maxrolls; i++) {
int sum = sumdice (NDICE);
/* you could do printf("%d\n", sum) here */
++sums[sum]; /* you could set a breakpoint here */
}

 

[Ian Collins]

* Finally, I prefer to avoid having extra variables. Rather than:

int sum; /* Sum of the dice. */
int i;

for (i = 0; i< maxrolls; i++) {
sum = sumdice (NDICE);
++sums [sum];
}

I'd write:

for (i = 0; i< maxrolls; i++)
++sums[ sumdice(NDICE) ];

On the other hand, some people prefer that one statement should
do only one thing, and that also makes debugging easier: you can
insert a printf to examine the return value of sumdice, and in a
debugger you can pause the execution after calling sumdice but before
incrementing sums (with a breakpoint or by line-by-line execution).

I'd just limit the scope of the variable to the loop:

int i; /* in case you're stuck to C90 */

for (i = 0; i< maxrolls; i++) {
int sum = sumdice (NDICE);
/* you could do printf("%d\n", sum) here */
++sums[sum]; /* you could set a breakpoint here */
}

An important point for the OP there is by declaring sum within the loop,
the name can be reused elsewhere. This reduces the chance of running
out of good names!

 

[Stefan Ram]

some people prefer that one statement should do only one thing (...)
for (i = 0; i < maxrolls; i++) {
int sum = sumdice (NDICE);
/* you could do printf("%d\n", sum) here */
++sums[sum]; /* you could set a breakpoint here */
}

The above for statement cannot be an example for this,
because it does not do only one thing.

 

[Ben Bacarisse]

* Finally, I prefer to avoid having extra variables. Rather than:

int sum; /* Sum of the dice. */
int i;

for (i = 0; i < maxrolls; i++) {
sum = sumdice (NDICE);
++sums [sum];
}

I'd write:

for (i = 0; i < maxrolls; i++)
++sums[ sumdice(NDICE) ];

On the other hand, some people prefer that one statement should
do only one thing, and that also makes debugging easier: you can
insert a printf to examine the return value of sumdice, and in a
debugger you can pause the execution after calling sumdice but before
incrementing sums (with a breakpoint or by line-by-line execution).

I'd just limit the scope of the variable to the loop:

int i; /* in case you're stuck to C90 */

for (i = 0; i < maxrolls; i++) {
int sum = sumdice (NDICE);
/* you could do printf("%d\n", sum) here */
++sums[sum]; /* you could set a breakpoint here */
}

That's a perfectly reasonable point of view. I feel I should add,
though, that I've never felt at any disadvantage writing the way I do.
The debugger has usually been able to tell me what I need to know and
there are probably other factors that mean I don't feel the lack of
printfs in situations like the above. I use them, but normally while
investigating failed tests of components. I.e. I might print from
sumdice while testing it, but once that's done I would not worry about
the particular combination in question.

 

[88888 Dihedral]

Ben Bacarisseæ–¼ 2012å¹´9月8日星期六UTC+8上åˆ11時06分45秒寫é“：

On 2012-08-29 14:27, Ben Bacarisse wrote:

* Finally, I prefer to avoid having extra variables. Rather than:

int sum; /* Sum of the dice. */
int i;

for (i = 0; i < maxrolls; i++) {
sum = sumdice (NDICE);
++sums [sum];
}

I'd write:

for (i = 0; i < maxrolls; i++)
++sums[ sumdice(NDICE) ];

On the other hand, some people prefer that one statement should

do only one thing, and that also makes debugging easier: you can

insert a printf to examine the return value of sumdice, and in a

debugger you can pause the execution after calling sumdice but before

incrementing sums (with a breakpoint or by line-by-line execution).

I'd just limit the scope of the variable to the loop:

int i; /* in case you're stuck to C90 */

for (i = 0; i < maxrolls; i++) {

int sum = sumdice (NDICE);

/* you could do printf("%d\n", sum) here */

++sums[sum]; /* you could set a breakpoint here */

}

I like your code to generate the normal distribution without any floating
point operations.

This is very helpful for 8-16-32 bit CPUs with slow floating point operations.

You can even write your own uniform distribution by linear feed back
registers with some randomized seeds.

Anyway there's a theorem in statistics that
any continuous real random variable distribution can be transformed
from the unit distribution in [0,1), but this requires floating point
operations.

 

[Ben Bacarisse]

I like your code to generate the normal distribution without any floating
point operations.

Do you mean you would like it to? As it is, it generates a binomial
distribution. (It's not my code, by the way, I was merely commenting on
it.)

This is very helpful for 8-16-32 bit CPUs with slow floating point operations.

Extending dice-rolling to approximate a normal distribution (in effect
relying on the central limit theorem) is not regarded as a good method.
I don't think this assessment changes even when floating point is
expensive.

 

[88888 Dihedral]

Ben Bacarisseæ–¼ 2012å¹´9月8日星期六UTC+8下åˆ7時28分16秒寫é“：

Do you mean you would like it to? As it is, it generates a binomial

distribution. (It's not my code, by the way, I was merely commenting on

it.)






Extending dice-rolling to approximate a normal distribution (in effect

relying on the central limit theorem) is not regarded as a good method.

I don't think this assessment changes even when floating point is

expensive.

The use of linear feedback register to generate a sequence of
non-zero numbers of 24 to 32 bits can be very fast.

This is the same as the problem of generating non-zero elements
in a finite field.

Then I think the sums of 16,64 or 256 terms should be good enough
in most applications which need a random variable in the normal
distribution.

There are other methods to generate normal random variables,
but the floating library of sin(x), cos(x), ln(x) and exp(x)
for x doubles may not be available or fast enough in
some platforms.

 

[lovecreatesbeauty]

VLAs are the most involved, I think, and they have been made optional in
C11. That's a crying shame to my mind, but that's how things have
panned out. No one who's implemented VLAs will remove the support, but
it makes them a second-class feature of the language.

The new introduced important "thread" feature is optional in C11.
The old VLA is made optional by C11 too.
This standard revision sucks.