views:

379

answers:

6

I can understand how a void** might look in memory, but I'm wondering if I'm using it quite right. Are there any fundamental flaws in what I describe below? For example, although I can say "it works for me", am I creating bad / unportable code in some way?

So I have an Asteroids clone. There are three entities that can fire bullets, the players (SHIP *player_1, SHIP *player_2) and the UFO (UFO *ufo). When a bullet is fired, it's important to know who fired the bullet; if it was a player, when it hits something their score needs to be incremented. So, the bullet will store what kind of entity it belongs to (owner_type) and also a pointer directly to the owner (owner):

enum ShipType
{
    SHIP_PLAYER,
    SHIP_UFO
};

typedef struct Bullet
{ 
    // ...other properties
    enum ShipType owner_type;
    void **owner;
} BULLET;

Then, when the player hits the button or the UFO sees a target, one of these functions will be called:

void ship_fire(SHIP **shipp)
{
    BULLET *bullet = calloc(1, sizeof(BULLET));
    bullet->owner_type = SHIP_PLAYER;
    bullet->owner = (void**)shipp;
    // do other things
}

void ufo_fire(UFO **ufop)
{
    BULLET *bullet = calloc(1, sizeof(BULLET));
    bullet->owner_type = SHIP_UFO;
    bullet->owner = (void**)ufop;
    // do other things
}

... they may be called, for example, like this:

ship_fire(&player_1);

Finally, when the bullet hits a target (such as an asteroid), we dereference the owner. If it's a ship, we can increment the score there and then.

void hit_asteroid(ASTEROID *ast, BULLET *bullet)
{
    SHIP *ship_owner;
    if (bullet->owner_type == SHIP_PLAYER && *bullet->owner != NULL)
    {
        ship_owner = (SHIP*)*bullet->owner;
        ship_owner->score += 1000;
    }
}

Does that seem a reasonable approach? Like I say, it works for me, but I only have a couple of months of C experience.

A final note: why do I not use a void* instead of a void**? Because I want to avoid dangling pointers. In other words, say that player_1 dies and is free'd, but their bullet keeps going and hits an asteroid. If I only have a void*, the hit_asteroid function has no way of knowing that bullet->owner points to de-allocated memory. But with a void**, I can validly check to see if it's NULL; if player_1 is NULL, then *bullet->owner will be NULL too.

EDIT: All respondents so far concur that using a void** probably isn't necessary here because I can avoid the dangling pointers issue (by just statically allocating the base object, for instance). They're correct and I will refactor. But I'm still kinda interested to know if I've used void** in a way that might break something e.g. in terms of memory allocation / casting. But I guess if no-one has thrown their hands in the air and declared it faulty, it at least resembles something that would technically work.

Thanks!

+2  A: 

Since you only have two possible types, I'd use a union for this sort of thing, like so:

typedef struct Bullet {
    enum ShipType owner_type;
    union {
        SHIP *ship;
        UFO *ufo;
    } owner;
} BULLET;

/* and then... */

void hit_asteroid(ASTEROID *ast, BULLET *bullet)
{
    SHIP *ship_owner;
    if (bullet->owner_type == SHIP_PLAYER && bullet->owner.ship != NULL) {
        ship_owner = bullet->owner.ship;
        ship_owner->score += 1000;
    }
}

Note that I didn't use the pointer-to-a-pointer scheme that you used. I'm not really convinced of the necessity of it, and the code I suggested doesn't require such a technique.

mipadi
Ah... nice! I've not tried using unions before and wasn't sure how they were used. Very good. Also, if I had, say, 100 different types of things that could shoot bullets, would you be more convinced of the pointer-to-a-pointer scheme?
Gavin Schultz-Ohkubo
+2  A: 

First off, check the union construct suggested by mipadi; that's a very readable and efficient way of dealing with polymorphism.

Closer to your snippet/question, at a quick glance, I don't see the need/use for the double indirection introduced by pointer-to-pointers. The whole logic would work the same if the arguments to xxxx_fire() methods were [direct] pointers to xxxx objects (and if the typecast etc. in the rest of the logic were to follow accordingly.

Pointers to pointers are useful when the value of the intermediate pointer may be changed at some point. For example if the underlying object is moved, or if it replace by a different object altogether (say a better-equipped ship part of a new level in game etc...)

Edit: (on the use of double indirection to manage "fleets" of objects which may be deallocated.
Responding to your comment, do not refactor so that the objects are not de-allocated (from memory) when they get killed/detroyed (as part of the game). Instead, look into something like the following, as this is indeed an example where the pointer-to-pointer construct helps a lot. Here's how it could work:

  • Upon game (or level) initialization, allocate an array of pointers big enough to contain as many pointers as the total number of objects the game may allocate, over time. Initialize all its values to NULL.
  • Introduce an int value index, which indicates the location of the next available (=unused so far) pointer in this array.
  • When a new object (UFO, Ship or what have you) gets created, four things happen:
    • new memory is allocated for the object per se
    • the address of this new memory is stored in the object pointer array (at the location indicated by the index)
    • the index gets incremented
    • the "world" only knows this object by way of the double indirection
  • when an object gets destroyed two things happen
    • the memory is freed
    • the pointer in the array is set to null
  • when accessing any objects the program does three things
    • first dereference (once) the pointer-to-pointer
    • check if this is null (if so this indicate the object doesn't exist anymore, the logic may decide to remove this reference from wherever it stored it, as so to not try again, but this is of course optional).
    • access the actual object by dereferencing the intermediate pointer (if it isn't NULL)

In insight, a short snippet in C language may have been more explicit; sorry I described this in words...

mjv
In this case, it is indeed because the underlying object may be changed: that is, it may be free()'d. I should probably refactor such that these particular objects aren't deallocated at all, since they're widely referenced and pretty intransient. But that was my reasoning.
Gavin Schultz-Ohkubo
+1  A: 

I would do like this:

enum _ShipType
    {
    SHIPT_PLAYER,
    SHIPT_UFO, //trailing , is good if you need to add types later
    };

typedef struct _Bullet
    { 
    // ...other properties
    struct _Bullet_Owner
        {
        enum _ShipType type;
        void* ship;
        }owner;
    } Bullet;

void ship_fire(Player* p)
    {
    Bullet* b = malloc(sizeof(Bullet));
    // ...other init
    b->owner.type = SHIPT_PLAYER;
    b->owner.ship = p;
    }

If there's only <constant> players, you would be better off having a dead flag for each and setting when they die. (And having them statically allocated.)

#define PLF_DEAD 0x1
//more stuff

struct _Player
    {
    long flags;
    //other data;
    }player_1,player_2;

Or you could have an array, or...

Edit: Nonconstant players, a horrifically overengineered solution:

typedef struct _PShip
    {
    long nweakrefs;
    void** weakrefs;
    //etc...
    }PShip;

PShip* PShip_new(/* args or void */)
    {
    PShip t;
    t = malloc(sizeof(PShip));
    t->nweakrefs = 1;
    t->weakrefs = malloc(sizeof(void*)*t->nweakrefs);
    //other stuff
    }
void PShip_regref(PShip** ref)
    {
    void** temp;
    temp = realloc((*ref)->weakrefs,(*ref)->nweakrefs);
    if(!temp){/* handle error somehow */}
    (*ref)->weakrefs = temp;
    (*ref)->weakrefs[(*ref)->nweakrefs++] = ref;
    }
void PShip_free(PShip* ship)
    {
    long i;
    for(i=0;i<ship->nweakrefs;i++)
        {
        if(ship->weakrefs[i]){*(ship->weakrefs[i]) = 0;}
        }
    //other stuff
    }

Alternatively, a reference count might work well, without the O(n) memory.

typedef struct _PShip
    {
    long refs;
    //etc...
    }PShip;

void Bullet_free(Bullet* bullet)
    {
    //other stuff
    if(bullet->owner.type == SHIPT_PLAYER)
        {
        if(--(((PShip*)(bullet->owner.ship))->refs) <= 0)
            {PShip_free(bullet->owner.ship);}
        }
    }

Also, neither of these is threadsafe.

David X
+1 ...for showing me the "horrifically overengineered" solution, I'm extremely grateful! (That is, you've given me another good reason to use constant players etc.)
Gavin Schultz-Ohkubo
@gavin thanks, but if you're doing something more sophisticated than WASD v Numpad (such as network multiplayer), you should look into the refcount idea; an arbitrary limit of, say, 8 players is guarrenteed to annoy someone eventually. (Q says Asteroids though so this probably is outside context.)
David X
+2  A: 

The linux kernel does this in an interesting way. It would be something like

/**
 * container_of - cast a member of a structure out to the containing structure
 * @ptr:        the pointer to the member.
 * @type:       the type of the container struct this is embedded in.
 * @member:     the name of the member within the struct.
 *
 */
#define container_of(ptr, type, member) ({                      \
        const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
        (type *)( (char *)__mptr - offsetof(type,member) );})


typedef struct Ship {
     void (*fire)(struct Ship * shipp);
     /* ...other methods...*/
} SHIP;

#define playership_of(shipp) container_of(shipp, PLAYERSHIP, ship)
#define ufoship_of(shipp) container_of(shipp, UFOSHIP, ship)

typedef struct PlayerShip {
    /* PlayerShip specific stuff ...*/
    SHIP ship;
    /*...*/
} PLAYERSHIP;

typedef struct UFOShip {
    /*...UFO stuff...*/
    SHIP ship;
    /*...*/
} UFOSHIP;

void ship_fire(SHIP * shipp)
{
     shipp->fire(shipp);
}

void player_fire(SHIP *shipp)
{
    PLAYERSHIP * ps = playership_of(shipp);
    BULLET *bullet = calloc(1, sizeof(BULLET));
    bullet->owner = shipp;
    // do other things
}

void ufo_fire(SHIP * shipp)
{
    UFOSHIP * ufos = ufoship_of(shipp);
    BULLET *bullet = calloc(1, sizeof(BULLET));
    bullet->owner = ufop;
    // do other things
}

UFOSHIP ufoship = { /*...*/ .ship = { .fire = ufo_fire } /* ... */ };
PLAYERSHIP playership = { /*...*/ .ship = { .fire = player_fire } /*...*/ };


/* ... */
ship_fire(&playership.ship);

Read the linux kernel source code for lots of examples of this tecnique.

Tim Schaeffer
+1 That is interesting - slightly beyond my skills or requirements for now, and I'll need to go away for a while to try to comprehend what container_of is actually doing. Looks like it lets you do some neat stuff though.
Gavin Schultz-Ohkubo
offsetof() gives the offset of a member of a structure, in bytes, from the beginning of the structure. container_of() does the inverse: given the pointer to the member (here, the embedded SHIP struct), it subtracts the offset of the member to get the pointer to the containing structure. It's fairly clear once you see a picture, but I don't know how to do that here. Perhaps SO will eventually add an HTML5 app that lets users draw diagrams (Hint, hint).
Tim Schaeffer
BTW, typeof() is a gcc extension, but you could do container_of() w/o it by adding the type as a parameter to the macro.
Tim Schaeffer
+1  A: 

If your bullet owners are frequently changed (e.g. deallocated), the pointer-to-pointer approach is suitable. The union solution does not address this concern directly; as presented, it does not support deallocating ships without touching the pointer on each of that ship's bullets. Of course, that may actually be a practical solution in some implementations, e.g. if you have a need to find all the bullets of a given player, you could maintain a linked list of them: a “next_bullet” pointer for each bullet and “last_bullet” pointer to the head of the list for each player.

And instead of allocating each bullet separately, I would also follow mjv's suggestion of pre-allocating some number of them and picking the next available one. In the linked list implementation, you could use the same “next_bullet” pointers to maintain one list of pre-allocated bullets not currently in use. The advantage of this approach is that you could easily allocate more if you ran out of them, instead of maintaining an array, i.e. if the list of available bullets is empty just add them to the list on demand. Similarly, put “expired” (exploded?) bullets back into the list of available ones and the amount allocated will automatically adapt to however many is required.

Another thing that comes to mind is that you might not need to know which particular UFO (or other enemy) owns a given bullet; just have a single pointer (e.g. SHIP **) for the owning player and set it to NULL for all non-player bullets. If this is not suitable, you could also consider storing the type of each owner in the beginning of owner struct itself, e.g.:

enum EntityType { TYPE_PLAYER_SHIP, TYPE_UFO_SHIP, TYPE_BULLET, };

struct GameEntity {
    enum EntityType type;
    // This struct is not actually used, it just defines the beginning
};

struct Ship {
    enum EntityType type; // Set to TYPE_PLAYER_SHIP when allocating!
    …
};

struct UFO {
    enum EntityType type; // Set to TYPE_UFO_SHIP when allocating!
    …
};

struct Bullet {
    enum EntityType type;  // Set to TYPE_BULLET when allocating!
    struct GameEntity *owner;
    …
};

struct Bullet *ship_fire (struct Ship *ship) {
    Bullet *b = get_next_available_bullet();
    b->owner = (struct GameEntity *) ship;
    return b;
}

void hit_asteroid (struct Asteroid *ast, struct Bullet *bullet) {
    if (bullet->owner && bullet->owner->type == TYPE_PLAYER_SHIP) {
        …
    }
}

Note that this trick relies on pointers to different types of structs being interchangeable, and the single enum being stored at the same offset in each type of struct. In practice these are not unreasonable assumptions, but I'm not certain that this behaviour is strictly guaranteed in standard C (however, e.g. struct sockaddr uses the same trick, and it's used by various POSIX networking functions like bind).

Arkku
+1 Some very useful advice in there, thanks!
Gavin Schultz-Ohkubo
+2  A: 

Even if you wanted to continue doing it the way you were, you don't need to use void ** (and shouldn't).

Although void * is a generic pointer type, void ** is not a generic pointer-to-pointer type - it should always point to a genuine void * object. Your code dereferences a SHIP ** or UFO ** pointer through an lvalue of type void ** - that's technically not guaranteed to work. (This happens when you do (SHIP*)*bullet->owner).

However, the good news is that you could continue to use the double-pointer method, using a plain void * to do the job. void * can happily store a pointer-to-a-pointer (because that, after all, is just another kind of pointer). If you change owner to void *, then in ship_fire you would do this:

bullet->owner = shipp;

and in hit_asteroid you would do this:

ship_owner = *(SHIP **)bullet->owner;

In general, the rule for working with pointer casts is: First cast the pointer back to the pointer type that you know it really is, then dereference.

caf
Good answer. I think this really gets to the questioner's concern about whether his design is a good idea.
jm
Ah, I get it, nice. I assume the void** still works for me only because I got lucky with my specific implementation and compiler. I'll try yours out.If void** isn't a generic pointer-to-a-pointer and thus isn't good for dereferencing directly, then is a void** only good for representing an array of void pointers? But probably that's a whole new question for SO...
Gavin Schultz-Ohkubo
That's right - `void **` is used for dealing with real `void *` objects through a layer of indirection (which could be an array of `void *` objects, or an output parameter or similar).
caf
Though the other answers contain a lot of good advice for me, as jm points out you've answered the heart of my question about void**, so a winner is you.
Gavin Schultz-Ohkubo