New C project i'm working on

[parsonsb]

Code: Select all

/*******************************************************************************
 * Simple dice rolling program. Takes input in the form of number of dice,
 * number of sides on dice, and number of times to roll. It then rolls the
 * dice and prints the output and input with a timestamp to a specific
 * file.
 * Author: Bryan Parsons
 * Date: 5-18-07
 * Email: ***********
 ******************************************************************************/
#include <time.h>
#include <stdlib.h>
#include <stdio.h>

int Roll(int sides)
{
    int x = srand(time(NULL));
    return x%sides+1;
}

int main ()
{
    int sides;
    int rollnum;
    int times;

    printf("Enter die type and number. Enter in format of 1d6 to roll 1 six sided die: ");
    if(scanf("%dd%d", &rollnum, &sides)!=2)
    {
        printf("Illegal input.\n");
        return EXIT_FAILURE;
    }
    return EXIT_SUCCESS;

}


so the program description is in the header, however on line 16, the one dealing with srand(time(NULL)); give me the error: dicerolling.c:16: error: void value not ignored as it ought to be


and i can't figure out what that means or how to fix it

[crazyjimbo]

srand returns void, i.e. it doesn't return anything. It only sets the random seed. To get a random number, call

Code: Select all

srand ( time(NULL) );
x = rand();


[EvanED]

And only seed the generator once on startup, not before each time you use it. (In other words, don't do it in Roll().)

[parsonsb]

ah thanks

[parsonsb]

^^ why not

[evilbeanfiend]

probably not terribly important assuming this is for a game but just in case its for some sort of noise simulation:

depending on the implementation rand can be a pretty poor prng, correlation in the lower order bits is especially a problem.

also using % to limit the range of the roll will mean you wont necessarily have a flat distribution (for small sided dice it will be close though anyway)

[Rysto]

If you seed the random function every time you use it, it ceases to be random.

(Well, ok, it wasn't random to begin with, but seeding every time you use it makes it very non-random).

[evilbeanfiend]

If you seed the random function every time you use it, it ceases to be random.

(Well, ok, it wasn't random to begin with, but seeding every time you use it makes it very non-random).

ya - he never called it in the orignal code though so thats alright

[EndangeredMassa]

Right. If you reset the seed before every roll, then you are picking the first number in N separate psuedorandom sequences, which is not guaranteed to be psuedorandom. So, you should make sure that you seed it at the beginning of the program and only once.

[Yakk]

Code: Select all

/*******************************************************************************
 * Simple dice rolling program. Takes input in the form of number of dice,
 * number of sides on dice, and number of times to roll. It then rolls the
 * dice and prints the output and input with a timestamp to a specific
 * file.
 * Author: Bryan Parsons
 * Date: 5-18-07
 * Email: ***********
 ******************************************************************************/
#include <time.h>
#include <stdlib.h>
#include <stdio.h>

struct ManyRoll {
    int sides;
    int count;
};

ManyRoll GetUserRolls(bool* error) {
    ManyRoll tmp = {0};
    if (error && *error) return ManyRoll();
    printf("Enter die type and number. Enter in format of 1d6 to roll 1 six sided die: ");
    if(scanf("%dd%d", &tmp.count, &tmp.sides)!=2) {
        if (error) *error = true;
        return ManyRoll();
    }
    return tmp;
}

struct RollState {}; // stores the data we need for our RNG.

RollState* MakeRollState( int optional_seed ) {
    time_t seconds;
    if(optional_seed) {
        srand(optional_seed);
        return calloc(sizeof(RollState), 1);
    }
    time(&seconds);
    srand(optional_seed);
    return calloc(sizeof(RollState), 1);
}

int DoRoll(bool* error, RollState* roll, int sides)
{
    double x;
    if (error && *error) return 1;
    if ((!roll) || (sides<1)) {
        if (error) *error=true;
        return 1;
    }
    x = (double)rand();
    x = x / (double)RAND_MAX; // x is from 0 to 1, inclusive
    x *= (double)sides; // x is from 0 to sides, inclusive
    x = ceil(x);
    if (!x) x = 1;      // x is from 1 to sides, inclusive.
    // note that the die is very slightly biased towards rolling 1.  The degree is undetectable.
    return (int)x;
}

struct RollBuffer {
  int* begin;
  int* end;
};

void DoManyRoll( bool* error, RollBuffer buff, RollState* roll, int sides) {
    if (error && *error) return;
    if ((buff.end < buff.begin) || (sides < 1)) {
        if (error) *error=true;
        return;
    }
    {for (int* it = buff.begin; it != buff.end; ++it) {
        *it = DoRoll(error, roll, sides);
    }}
    return;
}

int TotalManyRoll( bool* error, ManyRoll roll_desc, RollState* roll ) {
    // error checking skipped.  Add error checking before implementing
    int* buff;
    int total;
    buff = calloc(sizeof(int),count);
    RollBuffer b = {buff, buff+count};
    DoManyRoll( error, b, roll, roll_desc.sides );
    total = 0;
    {for(int* it = b.begin; it != b.end; ++it ) {
        total += *it;
    }}
    free(buff);
    return total;
}
int main ()
{
    struct ManyRoll roll_desc;
    struct RollState* roll_state;
    bool error;

    error = false;
    roll_state = MakeRollState(0);
    roll_desc = GetUserRolls(&error);
    int total = TotalManyRolls(&error, roll_desc, roll_state);
    if (error) {
        printf("Invalid input.\n");
        return EXIT_FAILURE;
    } else {
        printf("Your total is %d\n", total);
        return EXIT_SUCCESS;
    }
}


Untested code. I think the structure is a bit better.

Might be a bit of C99/C++isms scattered in there. Like I'm using a bool not a BOOL.

[adlaiff6]

I would change one thing:

Code: Select all

int DoRoll(bool* error, RollState* roll, int sides)
{
    double x;
    if (error && *error) return 1;
    if ((!roll) || (sides<1)) {
        if (error) *error=true;
        return 1;
    }
    x = (double)rand();
    x = x / (double)RAND_MAX; // x is from 0 to 1, inclusive
    x *= (double) (sides - 1); // x is from 0 to sides - 1, inclusive CHANGED
    x = ceil(x);
    x++;      // x is from 1 to sides, inclusive. CHANGED
    // note that the die NO LONGER (CHANGED) is very slightly biased towards rolling 1.  The degree is undetectable.
    return (int)x;
}

I also really, really dislike your unsure use of pointers ("if (error && *error)"). You really should go learn how pointers work, if you're that scared of them.

[EvanED]

There's a bigger reason than what has been mentioned for only seeding once though (unless that's what Rysto was saying).

Imagine that you now do somethnig like this:

Code: Select all

int RollNDice(int n, int sides)
{
    int total = 0, i;
    for(i=0 ; i<n ; ++i) {
        total += Roll(sides);
    }
}

time() returns seconds. This means that if it is called anywhere between, say, 0:00:00.000 and 0:00:00.999 it will return the same thing.

What's the chance that the n calls to Roll() will be done in different seconds? Pretty darn small. Which means that what you think are n random rolls aren't; you're almost universally going to get the same number for all of them.

[Rysto]

Mostly what I was getting at was that if the interval at which you call your random function is at all predictable, than your random numbers have become predictable as well.

This is just a generalization of what you've just said, really.

[Zanik221]

I also really, really dislike your unsure use of pointers ("if (error && *error)"). You really should go learn how pointers work, if you're that scared of them.

I'm not sure why that's a problem. He checks that the pointer is not null, then dereferences it to check it's contents. Would you rather be dereferencing a null pointer? You make baby jesus cry that way, you know.

[Yakk]

That is an error pattern.

1> You may pass a null error pointer. If so, the code runs, and any errors are ignored.

2> If the error pointer is non-null and true, the function exits immediately and does nothing.

3> If the error pointer is non-null and false, and an error occurs, it is set to true.

4> The return value of any function that exits on an error or because the error pointer is set is invalid.

It is a way of generating exception-like behavior without exception-like syntax and exception-like unpredictable code flow.

For a public project that uses this style, see the IBM ... ICU? ... project.

[evildave]

Another small thing to keep in mind about rand().

A lot of implementations of rand() vary in a very big way. Under gcc/linux, it is pretty good, and srand isn't even necessary (there's a daemon spinning random numbers all the time).

Under Microsoft's compiler, it's still a 16 bit value.

In other words, there are only 64K possible seeds.

You're better off shopping for a 'better' pseudo-random number generator, and including it in your project, rather than assuming all rand() implementations will be adequate. That's just a few web searches for 'random number generator' will turn up one that's reasonably quick and with an acceptable license.

Also, having control over as many instances of 'random' as you like has benefits over a single, global rand(). For instance, you might want to encrypt something 'important', without resetting the sequence you're using to shuffle cards and roll dice.

In gaming, you should also 'kick' the random number generator by invoking rand() (or whatever your generator is) every main cycle, and at infrequent oddball events to get sort-of better randomness. Every cycle of delay a user spends reading something, and events such as receiving certain packets can 'roll the dice' on the generator, making it more 'random-like', and less 'pseudo-random-like'.

An extreme example of this (which I half remember as a story and have not personally verified) was for an on-line poker game. Someone allegedly worked out that they were using Microsoft's 15 bit generator on a Windows server, and it was seeded at the start of the game for that instance of the game. What this meant was they only had to check seeds for the time(NULL) result at the server +/- a couple of seconds and determine what the shuffle would look like (apparently, they knew or reverse-engineered the shuffle algorithm in use, too). From this, they could determine what everybody's hand would be in the game quickly enough for it to be an advantage (i.e. after the first couple of hands).

As for the pointer checks, use 'assert()'. That's exactly what it's for.

[evildave]

Many C runtime libraries have a 'hook' for memory allocation failures (especially C/C++ runtime libraries with '_set_new_handler' handlers). You may as well use that, if it's available. One place to hook covers EVERY possibly memory allocation failure and usually lets you try to free something up to 'fix it'.

If not, make your own memory testing macros/functions. You can centralize all of this memory checking to one kind of test with one kind of handler (this is usually fatal, after all) that tells you about the memory problems. You can extend them to detect buffer underruns/overruns, get lists of allocated things, etc.

// MyMemory.h
#undef malloc
#undef calloc
#undef realloc
#undef free
// etc.
#define malloc(size) MyMalloc(size,__FILE__,__LINE__)
#define calloc(nellements,size) MyCalloc(nellements,size,__FILE__,__LINE__)
#define realloc(ptr,size) MyRealloc(ptr,size,__FILE__,__LINE__)
#define free(ptr) MyFree(ptr,__FILE__,__LINE__)
// etc.

The thing I guess everybody is getting at (or should be getting at) is that something as trivial as dice rolling shouldn't even make an error, if there's any way possible that you can engineer it out.

If it runs out of memory, that will in all probability be 'totally fatal to the max', with no recovery possible. You may as well have a single handler to tell you you're out of memory, and either try to free things up or die quietly with an appropriate message, than miss one test some place and die horribly in some less scrutable manner. It will also shrink your code a bit, and simplify many things, like this 'error' status doohickey.

[EvanED]

Another small thing to keep in mind about rand().

A lot of implementations of rand() vary in a very big way. Under gcc/linux, it is pretty good, and srand isn't even necessary (there's a daemon spinning random numbers all the time).

That doesn't feed rand() though; that's the entropy generator, which feeds /dev/random and /dev/urandom. It's also "truly" random.

If you use rand(), it uses the same pseudo-random generator that needs a seed:

Code: Select all

evan@localhost ~/temp $ cat rand.c
#include <stdio.h>
#include <stdlib.h>

int main()
{
  printf("%d\n",rand());
  return 0;
}
evan@localhost ~/temp $ gcc rand.c -o rand
evan@localhost ~/temp $ ./rand
1804289383
evan@localhost ~/temp $ ./rand
1804289383
evan@localhost ~/temp $ ./rand
1804289383
evan@localhost ~/temp $ ./rand
1804289383
evan@localhost ~/temp $ ./rand
1804289383
evan@localhost ~/temp $ ./rand
1804289383
evan@localhost ~/temp $ ./rand
1804289383
evan@localhost ~/temp $ ./rand
1804289383

(There's also a better "pseudorandom" number generator in Windows, though curiously, apparently no entropy generator in the same sense of Unix. The pseudorandomness of RtlGenRandom is of a qualitatively different form than the pseudorandomness of rand(); see the second comment here for how it works.)

An extreme example of this (which I half remember as a story and have not personally verified) was for an on-line poker game. Someone allegedly worked out that they were using Microsoft's 15 bit generator on a Windows server, and it was seeded at the start of the game for that instance of the game. What this meant was they only had to check seeds for the time(NULL) result at the server +/- a couple of seconds and determine what the shuffle would look like (apparently, they knew or reverse-engineered the shuffle algorithm in use, too). From this, they could determine what everybody's hand would be in the game quickly enough for it to be an advantage (i.e. after the first couple of hands).

This is probably not what you're talking about (they don't state that it's an MS thing), but read the section "On To Poker, Or How To Use A Random Number Generator Badly" of How We Learned To Cheat At Online Poker.

As for the pointer checks, use 'assert()'. That's exactly what it's for.

It may not be what it's for. Null might be a perfectly valid value to pass in. Using it as an optional parameter essentially. For instance, I use that for optional out parameters.

(This is leaving alone any discussion of whether this way makes sense for dealing with errors. I don't know enough C idioms to feel comfortable making any judgment there.)

[Yakk]

Another small thing to keep in mind about rand().

A lot of implementations of rand() vary in a very big way. Under gcc/linux, it is pretty good, and srand isn't even necessary (there's a daemon spinning random numbers all the time).

Under Microsoft's compiler, it's still a 16 bit value.

In other words, there are only 64K possible seeds.

You're better off shopping for a 'better' pseudo-random number generator, and including it in your project, rather than assuming all rand() implementations will be adequate. That's just a few web searches for 'random number generator' will turn up one that's reasonably quick and with an acceptable license.

This is why I created the "random state" struct, even though I didn't use it. Rather than use global variables, you pass in a random state explicitly.

As for the pointer checks, use 'assert()'. That's exactly what it's for.

Assert is for checking things that you don't want to check in a release build, because it should never happen.

Null pointers are sometimes a valid thing to pass into a function. Calling assert when a pointer being null is a valid option is ... well, stupid.

A truely robust program checks for failure of allocation, and deals with it, and either adapts or shuts down cleanly.

If it runs out of memory, that will in all probability be 'totally fatal to the max', with no recovery possible. You may as well have a single handler to tell you you're out of memory, and either try to free things up or die quietly with an appropriate message, than miss one test some place and die horribly in some less scrutable manner. It will also shrink your code a bit, and simplify many things, like this 'error' status doohickey.

The bool in this case is being used for parsing user input, as well as any other purpose. It is a pretty general pattern with many uses.

[evildave]

"Null pointers are sometimes a valid thing to pass into a function. Calling assert when a pointer being null is a valid option is ... well, stupid. "

Stupid? Any convention where it CAN be NULL is a crash waiting to happen. You WILL miss obscure cases when the code gets bigger. The code's already polluted with 'error handlers' checking for a NULL pointer. Is it so unreasonable to EXPECT the pointer to point at something valid?

Assert checks for implementation and calling problems, and a NULL pointer to a boolean is an obvious coding error, not a 'pattern'.

[EvanED]

"Null pointers are sometimes a valid thing to pass into a function. Calling assert when a pointer being null is a valid option is ... well, stupid. "

Stupid? Any convention where it CAN be NULL is a crash waiting to happen. You WILL miss obscure cases when the code gets bigger. The code's already polluted with 'error handlers' checking for a NULL pointer. Is it so unreasonable to EXPECT the pointer to point at something valid?

Assert checks for implementation and calling problems, and a NULL pointer to a boolean is an obvious coding error, not a 'pattern'.

What?

Everything from the C function time() to Windows NT functions to generic advice on when to pass by pointer vs reference support using NULL pointers as optional parameters.

What exactly is your objection with Yakk's code? Is it something specific to his error-handling idiom?

[djn]

I don't know if these are even available on linux, but I'd check.
FreeBSD has two other random generators available in addition to rand():
random() (seeded with srandomdev()), and
arc4random() (which uses /dev/urandom to initialize itself).

random() is better than rand(), and arc4random() is better than both.
arc4random() is slower than random() which is slower than rand().
arc4random() uses entropy from /dev/random, if you need crypto-grade randomness there is a limit to how much you can pull out per second before it gets (theoretically) predictable.

[MrSparkle]

Another small thing to keep in mind about rand().

A lot of implementations of rand() vary in a very big way. Under gcc/linux, it is pretty good, and srand isn't even necessary (there's a daemon spinning random numbers all the time).

Under Microsoft's compiler, it's still a 16 bit value.

In other words, there are only 64K possible seeds.

I'm not saying rand() is really a good PRNG, but I feel its being demonized quite a bit here.

On a lot of systems (including MS compilers and Intel compilers) rand() is implemented as a "fast rand." The seed passed to it is indeed 32-bits, and all 32-bits are used. It does a relatively simple calculation, and then the low 15-bits are returned. Its important to note that this does not mean there are only 2^15 effective seeds. The internal math is still 32-bit, just not all 32-bits are returned.

Also very important to note is that even though only 16-bits are returned, using *any* seed value (except for 0, which is usually specifically prohibited by the code(IE if you seed with 0, it just uses 1 instease)) the period of rand() implementations should be (and is in the case of MS and Intel compilers) 2^32.

Here is the usual implmentation of fast rand():

Code: Select all

int holdrand = 1;
void srand( int seed )
{
        holdrand = seed;
}

int rand()
{
        return(((holdrand = holdrand * 214013 + 2531011) >> 16) & 0x7fff);
}


The constants in there are chosen very carefully for mathematical reasons. The important thing to note is that 2531011 is relatively prime to 2^32. This is what guarantees that the period of the function is at least 2^32.

There's a lot more to learn about RNGs than this, of course. But don't hate on rand() so much. Sure its predictable, but that doesn't mean its not "random" enough for most applications.

[Yakk]

Null pointers are sometimes a valid thing to pass into a function. Calling assert when a pointer being null is a valid option is ... well, stupid.

Stupid? Any convention where it CAN be NULL is a crash waiting to happen.

All pointers CAN be null, by the language standard. The question is, is it allowed by the function's contract or not?

The contract I was using was:
The error pointer can be null. If it is null, then you won't be told if an error happens, but execution will proceed regardless.

The random engine state pointer cannot be null. If it is null, it is an error.

The user may not enter invalid data. If they enter invalid data, it is an error.

Errors are passed out via the error boolean.

The contract of "no pointer to this function can be null" is also a valid contract to ask of the callers. In that case, assert guards on the pointers being passed in is a good idea. For more robust code, Verify guards that check in both debug and release can also be useful.

(Verify is an assert that evaluates in both debug and release, and returns the result of the Verification. You can use it as follows:

Code: Select all

// bar must be non-null, and *bar must be non-zero.
void foo(int* bar) {
  if (!Verify(bar && *bar)) {
    SignalInternalErrorAndExit( __FILE__, __LINE__, "invalid bar");
  }
  // code
}


You WILL miss obscure cases when the code gets bigger.

Those cases will be missed if I fail to assert non-null, or if I fail to check for a null pointer.

Personally, I prefer to write in C++, where I can do things like:

Code: Select all

template<typename T>
struct non_null_ptr {
  T* t;
  T* operator*() {...}
  T const* operator*()const {...}
  ...
};


and write interfaces that assert for me. That was the body of a function can be much cleaner, with as much assertion as possible shoved off into code that the compiler automatically writes for me. :)

But I'm weird that way.

The code's already polluted with 'error handlers' checking for a NULL pointer. Is it so unreasonable to EXPECT the pointer to point at something valid?

No, that isn't unreasonable. Your function's contract with it's callers can be "this pointer cannot be null, and must be valid". That is an acceptable contract, especially for quick-and-dirty small programs.

My choice to allow for some null pointers in my contract is a choice, one of many valid ones. This isn't a religious issue with me, it is just a coding pattern I found useful.

Assert checks for implementation and calling problems, and a NULL pointer to a boolean is an obvious coding error, not a 'pattern'.

Assert checks are for things that should never happen, and you only want to check in debug mode because of performance issues.

Writing code that is explicitly more robust is not a coding error.

Writing code that uses pointer-as-null as an optional parameter is not a coding error.

As noted, the C standard library uses "optional pointers, with null meaning don't use it". So I'm not just making things up -- this convention is as old as the C language, and probably older.

[evilbeanfiend]

yakk is right sometimes NULL is necessary e.g. end of a linked list

[evildave]

I agree that there are many ways to skin a cat, but...

There is no such thing as an 'optional' error.

If you discover that this 'optional' error pointer is being initialized and passed into things on the stack EVERYWHERE, you might consider making it a (gasp!) 'global', or as part of the structured dice state. It'll be there because the app loaded. You could wrap that with a 'GetDiceError()' function in the library and hide the existence of that global, and not have to initialize it and pass it around everywhere. Then the caller can check for errors or perilously ignore them as they wish.

In this particular case, I still maintain there's no reason to fail at all. The only true failure possible is a memory allocation failure, and I already went into how to make those failure tests go away. The other one's a parameter parsing issue, and that might be better dealt with long before calling the die roller.

Writing code that can not possibly fail is writing robust code.

Writing code that has failure built into it is not writing robust code.

For every 'failure mode' you create, you also create MANY places where that failure must be tested, and the code to handle the errors must grow exponentially, or you may as well have not have manufactured an error condition in the first place.

Yes, the C runtime library returns some NULL pointers in certain cases, and those need to be handled. Yes, many runtime libraries accept a 'NULL' as parameter meaning 'default this'. NO, you do not have to follow this terrible practice in your own code. NULL means 'bug', just as a pointer to '43' is a bug, or any pointer outside your normal addressing space is a bug. You shouldn't be passing around bad pointer values as a matter of course. A pointer is NOT a scalar or an enumeration, no matter how many people pretend it is.

A container is indeed a specific case where NULL is commonly used, and I have used it, too. There, you're saying a leaf in a tree has nothing. You could still do without NULL (for instance std::list<> in C++ doesn't use it), but as a matter of writing compact data it's used.

This IS a 'religious issue' for me. I've worked on too many large teams with marginal programmers in them (even ONE would be too many, but EVERY team has marginal members), and giving someone else rope to hang themselves with usually means that *I* get hung, and then don't leave work for days at a time near the end of a development cycle because these same people don't know how to find and fix their own memory smashing bugs, either.

I'm also very intolerant of people playing the latest 3D shooter for a four hour lunch, and other trivial little 'bad habits' like that. If you want to play games, THEN TEST OUR GAME.

Finding a bad habit and stamping it out early in the development cycle means I get enough sleep, and am less cranky overall.

Besides, you know any code you post here, no matter how trivial, is going to get an 'anal probe'. To be fair, I suppose I could post something 'truly horrific' and defend it to the bitter end....

[Yakk]

I agree that there are many ways to skin a cat, but...

There is no such thing as an 'optional' error.

Yes there are. As an example, I might call a function that could fail, but not care if it fails. So long as it doesn't break the program state and leaves on failure.

In that case, I would call the function with a null error pointer.

If you discover that this 'optional' error pointer is being initialized and passed into things on the stack EVERYWHERE, you might consider making it a (gasp!) 'global', or as part of the structured dice state. It'll be there because the app loaded.

Thread-local would be better, but even that isn't ideal. I want to give the client explicit control over what to do with errors, when to handle them, when to ignore them, and I prefer my functions to be as "functional" as is reasonably possible (ie, minimize side-effects).

In C++, you can do a significantly better job of the error parameter because of the automatic code generation abilities of it's struct/class syntax.

You could wrap that with a 'GetDiceError()' function in the library and hide the existence of that global, and not have to initialize it and pass it around everywhere. Then the caller can check for errors or perilously ignore them as they wish.

This does a number of bad things.

First, it introduces global state to your program. Global non-thread-local global state. Global state that is read and written in multiple locations throughout your code base.

That's pretty bad.

On top of that, it extends the interface of your function in a custom way. Instead of a simple pattern that can be worked out from the interface of the function, we now have a collection of 3 functions which interact using implicit connections between them.

In this particular case, I still maintain there's no reason to fail at all. The only true failure possible is a memory allocation failure, and I already went into how to make those failure tests go away. The other one's a parameter parsing issue, and that might be better dealt with long before calling the die roller.

The RNG state could be set up wrong.

If you are using a entropy-based RNG, you could run out of enthropy to generate new RNGs.

The error pointer is does not just only exist to be set: if the error pointer points to true, no code will be executed by a function that is called. This lets you write out a bunch of code and have it stop doing stuff at the moment the first error occurs.

One can do this using exceptions, but exceptions have their own problems: they are implicit, they can't be signatured correctly in C++, they cause unpredictable code-flow, they require careful use of RAII, and they don't work very well in C.

Writing code that can not possibly fail is writing robust code.

Writing code that has failure built into it is not writing robust code.

Assuming code cannot fail is not a robust code practice. Code that is robust is code that deals with failures in a well defined and tidy manner.

Every failure that is ignored is a possible code path quirk that has to be understood, possibly tested, and delt with.

Note that the error being set to true is optional. The code should seek to behave as well as it can, but when it is given garbage it can notify the calling function as well as behaving as well as it can.

Yes, the C runtime library returns some NULL pointers in certain cases, and those need to be handled. Yes, many runtime libraries accept a 'NULL' as parameter meaning 'default this'. NO, you do not have to follow this terrible practice in your own code. NULL means 'bug', just as a pointer to '43' is a bug, or any pointer outside your normal addressing space is a bug.

This isn't consistent with the C standard, in which NULL is a well defined pointer. A pointer outside of your memory space isn't well defined in it's operation.

You can treat NULL pointers as bugs: that is an acceptable idiom. Claiming that any other idiom about NULL pointers is WRONG is ridiculous. :)

Speaking of ridiculous, vi or emacs? How many characters wide should tabs be? Do you put {} on the line-of the if, or on the next line?

I'm just curious what other religious beliefs you hold. :)

You shouldn't be passing around bad pointer values as a matter of course. A pointer is NOT a scalar or an enumeration, no matter how many people pretend it is.

No, pointers are not scalar values. I'm not using them as scalar values. I'm using them as a reference to storage that can refer to "nothing" optionally in one case, and in the other case as a reference to storage that must exist.

This being C code, I can't distiguish between them strongly in the language syntax, so I am forced to distinguish between them using convention.

Besides, you know any code you post here, no matter how trivial, is going to get an 'anal probe'. To be fair, I suppose I could post something 'truly horrific' and defend it to the bitter end....

Sure, you are free to put forward your religious beliefs about coding. I, however, am under no obligation to tolerate your religion.

I claim there is more than one idiom. They have advantages and disadvantages.

What I displayed above was a defensively coded implementation using an error-handling idiom that I have seen used usefully, that I have used usefully, and that generates some nice properties that other error handling idioms lack.

It isn't the be-all the end-all error handling idiom, but it is still a useful one.

Regardless ofy your personal religious beliefs.

If you want to convince me, you could try not argueing from a religious fanatic standpoint. :) If you want to amuse me, continue.

[evildave]

It's ALL about the entertainment.

That's a core tenet.

Secondly... a multithreaded 'simple dice rolling' program?

You gots to be kiddin' me.

If your dicey state (rand#, error, etc.) is in a struct with the error, then you'll have one error per set of dice kinds of things.

If your dicey state is globals, then a global error handler.

If you want 'thread local' global, that's up to you, but I find all the thread locking to be tiresome, and stick to cooperative multitasking for the bulk of the application, and reserve the threads for 'special' tasks, like pumping the sound queue or polling for and sending packets to/from queues. Even the render (when multithreaded) should be working off a copy of the game state that gets exchanged so you can be preparing the next frame without blocking it.

Code that is so tangled with threading that invocations of a dice-rolling function need protection... well that's just not a good model for success. I wouldn't write that sort of gaming function for anything but the ONE thread that controlled the game to operate on. That means no conflict AND no worries about threading.

In short, ideally what happens in a thread should have very little interaction with what happens outside of it, and I certainly have no reason to add multi-threading assumptions if they aren't called for, and they are SELDOM called for. I think you're just making up excuses for having a pointer to be contrary, now.

Something that 'rolls dice' is sounds suspiciously turn based, and even if it is real-time, there is NO compelling argument to be made for threading.

Even on a server, you're best served spawning a process to run the server end of the game with its participants from whatever lobby it has.

If you go all multi-threaded on the game model, you WILL regret it.

[djn]

I'm not sure how your dislike of threading is relevant to the issue of whether NULL can be an acceptable value for a pointer argument or not?

[evildave]

Because the case against stuffing the error state into a global was that it was 'not thread safe'.

[adlaiff6]

If you want NULL to specify something, it should be checked for, and have a unique procedure associated with it. The way it's written, passing error=NULL is no different from error=FALSE, so there is no reason to pass a NULL pointer. If there is no reason to pass a NULL pointer, it shouldn't be passed, and shouldn't need to be checked for.

[evilbeanfiend]

this is hella offtopic

[djn]

If you want NULL to specify something, it should be checked for, and have a unique procedure associated with it. The way it's written, passing error=NULL is no different from error=FALSE, so there is no reason to pass a NULL pointer. If there is no reason to pass a NULL pointer, it shouldn't be passed, and shouldn't need to be checked for.

The physical, if that's the word, value of NULL and FALSE is normally indistinguishable, so setting it to FALSE doesn't really get you anywhere. At least as long as you have to pass a pointer and not a value.
Imagine that the function expects a pointer to where it should store the error values, alternatively NULL if it shouldn't bother. Replacing NULL with FALSE would not help you in any way.
One alternative would be to always pass a valid pointer, pointing at 1 if you want it used and 0 if not. Of course, that's exactly as inconvenient as not having the argument be optional in the first place.

[Yakk]

If you store errors in global variables, then you have the implicit rule that one thread and one thread only can ever roll dice.

It also means that if you call a die rolling function, you must record and deal with any errors before you call another die rolling function. Practically, that means every roll of the dice requires a bunch of bulky error handling code right after it. This is a bad idea.

If you allow errors to be stored and deferred, you can route the error-handling code away from the main code flow. This is the idea behind exceptions. The "shared error pointer that future code reads and skips execution" gives you the exception-like "deal with errors elsewhere" advantages together with explicit code-flow constructs. It doesn't place implicit restrictions on the threadedness of your code. It doesn't require creating 3 functions for every function, linked by convention via naming.

And, as I noted, you can ignore the error by simply setting the error pointer to null, and hope for the best. :)

[evildave]

Except we're talking about C, not C++. No exceptions.

And there ain't no point to running game rules in multiple threads. Every project I've seen that has done it has been a catastrophe, and every project that recovered from that abyss was because the threading was abandoned. There are just too many ways for a non-casual simulation's parts to interact together, and too many places to deadlock or block to the point where the single-threaded code is faster, no matter how many processors you're using.

If you DO handle the errors, they're no more bulky with the global than the pointers, and substantially less so as you won't be declaring and passing around the pointer.

But that's beside the point. There should not be any error possible. Just a little tweaking of your design philosophy could eliminate any need for error checking because it can not cause any error.

For instance, instead of making a big list of die rolls, you could treat the Die roller as a 'generator'. There's no need for the code to call the random number generator umpteen times in a loop into a buffer, only to have another function iterate a buffer in another loop.

There's no particular need to parse and validate the command line input all the way in a dice function. Parse it in main where it's usually done. Then you don't need that error, either.

The memory allocation, well that can't fail, can it? You don't have a whole library of mark&sweep or cached memory managed thingamajigs to flush. If malloc fails, that's 100% fatal, probably indicating the arena was corrupted by a stray pointer (or you wanted billions and billions of rolls).

[djn]

Backing out of the error-related field and back into NULL-as-optional, how about this kid of code? (C, not C++)

Code: Select all

void roll_many(int num_rolls, uint8_t *res, struct timeval *time_spent) {
    struct timeval time_start, time_end;

    if (time_spent != NULL)
        gettimeofday(&time_start, (struct timezone*) 0);

    for(;num_rolls>0; num_rolls--) res[num_rolls-1] = (random() % 6)+1;

    if (time_spent != NULL) {
        gettimeofday(&time_end, (struct timezone*) 0);
        timersub(&time_end, &time_start, time_spent);
    }
}


In this specific case I might as well have wrapped the call in timekeeper functions, but assume that there is some reason for not doing so (e.g. that I only want to time some parts of a more complicated function). Also assume that gettimeofday() is expensive.
Is there any good reason for not doing it like this, and what's the alternative?

And, yes:
* I should have used clock() for profiling.
* I did the for loop like that mainly to reduce the line count in the example.

[evildave]

A proper C solution. The die-rolling things don't give a tinker's cuss about where the buffer came from and how it was allocated. Nothing makes an error except where you have complete control to deal with it.

Contains a verbose and a non-verbose output mode.

Code: Select all

/*
 * Dice Roll-O-Matic
 */
#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <time.h>

void GetRolls( int* aDice, size_t cDice, int sides );
int TallyRolls( const int* aDice, size_t cDice );
int ShowRolls( const int* aDice, size_t cDice );

/* Give command line help */
void HelpMe(void)
{
   printf("roll 1d6 [2d6...]\nEnter count of dice and number of sides; 1d6 rolls one six sided die\n");
   exit(1);
}

/* Parse the command line and test the functions */
int main(int argc, const char** argv )
{
   int bVerbose = 0;
   if( argc < 2 )
      HelpMe();
   srand(time(NULL));
   while( --argc )
   {
      int count;
      int sides;
      int* dieBuff;
      const char* pBuff = *++argv;
      if( *pBuff == '-' && pBuff[1] == 'v' || pBuff[1] == 'V' )
      {
         bVerbose = pBuff[2] == '-' ? 0 : 1;
      }
      else if(sscanf(pBuff, "%dd%d", &count, &sides)!=2)
      {
         HelpMe();
      }
      /* Allocate the buffer dynamically. */
      /* Most applications will want a fixed buffer sized for the roll */
      dieBuff = malloc(count * sizeof(int));
      if( NULL == dieBuff )
      {
         printf("Allocation failure.\n");
         exit(2);
      }
      {
         /* Profile and display results */
         clock_t began = clock();
         clock_t result;
         GetRolls( dieBuff, count, sides );
         result = clock() - began;
         if( bVerbose )
         {
            ShowRolls( dieBuff, count );
         }
         else
         {
            printf( "Total: %d\n", TallyRolls( dieBuff, count ) );
         }
         printf( "Time:%d.%03d seconds\n", result/CLOCKS_PER_SEC, (result%CLOCKS_PER_SEC)*1000/CLOCKS_PER_SEC );
      }
      free(dieBuff);
    }
   return 0;
}

/*
 * Roll the dice
 * sides: Number of sides per roll
 * aDice: Array to receive dice
 * cDice: Number of dice to place in array
 */
void GetRolls( int* aDice, size_t cDice, int sides )
{
/* Choose a method.  Add more if you like. */
#define METHOD 3
#if(METHOD==1)
   /* On some platforms, pre-cooking the division as a fp multiply is best */
   float randscale = sides * (1.0f/RAND_MAX);
   while( cDice-- )
   {
      *aDice++ = 1+(int)(randscale*rand());
   }
#elif(METHOD==2)
   /* Some random number generators favor the low end */
   float randscale = sides * (1.0f/RAND_MAX);
   while( cDice-- )
   {
      *aDice++ = 1+(rand()%(sides));
   }
#elif(METHOD==3)
   /* Some random number generators favor overall */
   while( cDice-- )
   {
      *aDice++ = 1+(rand()*(sides)/RAND_MAX);
   }
#endif
}

/*
 *  Count up the dice
 *  aDice: Array containing dice
 *  cDice: Number of dice to place in array
 */
int TallyRolls( const int* aDice, size_t cDice )
{
   int result = 0;
   while( cDice-- )
   {
      result += *aDice++;
   }
   return result;
}


/*
 * Show us the dice
 * aDice: Array containing dice
 * cDice: Number of dice to place in array
 */
int ShowRolls( const int* aDice, size_t cDice )
{
   int result = 0;
   while( cDice-- )
   {
      int die = *aDice++;
      printf( "%d ", die );
      result += die;
   }
   printf( "Total: %d\n", result );
   return result;
}


[evildave]

You'll probably want this one, too. It will tell you more about the distribution of the values, so you can be sure the rolls are reasonably fair.

Code: Select all

/*
 * Show us how many times each side got picked
 * aDice: Array containing dice
 * cDice: Number of dice to place in array
 * sides: Count of sides
 */
void ProfileRolls( const int* aDice, size_t cDice, int sides )
{
   int* tallyTab = calloc(sides,sizeof(int));
   if( NULL == tallyTab )
   {
      printf("Allocation failure.  Profile aborted.\n");
      exit(2);
   }
   {
      size_t remain = cDice;
      while( remain-- )
      {
         tallyTab[*aDice++ -1]++;
      }
   }
   {
      int side;
      int nDigits = 1+log10(cDice);
      printf( "Per-side results...\n" );
      for( side = 0; side < sides; ++side )
      {
         printf( "%*d ", nDigits, 1+side );
      }
      printf( "\n" );
      for( side = 0; side < sides; ++side )
      {
         printf( "%*d ", nDigits, tallyTab[side] );
      }
      printf( "\n" );
   }
   free(tallyTab);
}


[Yakk]

Why bother with any of these parameters:

void GetRolls( int* aDice, size_t cDice, int sides );
int TallyRolls( const int* aDice, size_t cDice );
int ShowRolls( const int* aDice, size_t cDice );

Just have global variables that the functions read and write to.

I'm aware that you can avoid having to have an error pointer in a simple function. I'm also aware that having an error pointer in some functions leads to some useful side effects: like your code being able to report back an error to the handler, instead of having to call exit() and end all program execution.

The cost of the habit of not changing or accessing global state, and allowing for a path for errors to propogate, in a small toy program has a pay off: when you are writing a larger program, your code won't get as tangled up in global state. Some function deep in the call tree won't call exit() on you, and instead will have a path to propogate errors out of it.

Asto the wonders of doing arguement unpacking in the same function as one does execution logic... :)

[parsonsb]

sorry for my lack of response to the thread i created, i've been kinda dead this week with mono