docopt / docopt.c

C-code generator for docopt language.
MIT License
318 stars 46 forks source link

Unit tests are broken #18

Closed kblomqvist closed 11 years ago

kblomqvist commented 11 years ago

Just realized that Tokens* tokens_new() wasn't just a rename from Tokens tokens_create()... @ffunenga I'm not sure why do we added that extra param of Tokens itself in to the argument list of the function, and why do we return a pointer:

Tokens* tokens_new(Tokens *ts, int argc, char **argv) {
    struct Tokens update = {argc, argv, 0, argv[0]};
    (*ts) = update;
    return ts;
}

If an update function is needed then let's add it separately.

ffunenga commented 11 years ago

The return value was thought more like a sucess/error code. If there is any error in the initialization, we can return NULL. But, in this case, it is not needed because this function only makes the struct initialization. Normally, an int is used for the return value.

Concerning the first argument, normally I instantiate new objects in C with this type of structure: first declaring the struct inside a function, then using the memory address of the struct to call methods. Example:

void examp_func(void) {
    ClassName object;

    /* the constructor can have other names like: classname_init or classname_create */
    classname_new(&object);
    classname_set_value(&object, 3);
    return;
}

Its only a matter of coding style. IMO, it is more important to initialize all the structs in the same way. If you feel the other style is better, then lets change it :)

Proposed solution for the function:

int tokens_new(Tokens *ts, int argc, char **argv) {
    struct Tokens update = {argc, argv, 0, argv[0]};
    (*ts) = update;
    return 0;
}    
kblomqvist commented 11 years ago

I agree that all structs should be initialized similarly. But, I don't understand why to use this "first declaring the struct inside a function" way of initialization within foo_new(). I would like to revert this as it was previously:

Tokens tokens_new(int argc, char **argv) {
    Tokens ts = {argc, argv, 0, argv[0]};
    return ts;
}

Tokens ts = tokens_new(argc, argv);

This way Token initialization looks like the use of new keyword in C++ and I feel it more convenient to use in this way. If the Token has to be reset later there could be a function:

Tokens* tokens_reset(Tokens *ts, int argc, char **argv) {
    (*ts) = tokens_new(argc, argv);
    return ts;
}