2
typedef struct
{
    int data1;
    float data2;
} packetType1;

typedef struct
{
     bool data1;
} packetType2;

typedef union
{
     packetType1 p1;
     packetType2 p2;
} packet;

struct
{
    int type;
    packet myPacket;
} message;

Now I make a message and pass the pointer to this message to a function. Inside this function I need to de-reference the message and take out the necessary data from it.

This data will depend on whether packet was filled with data of packetType1 type or of packetType2 type. Inside message , the integer variable type can contain the value 1 or 2 only, telling that packet inside message is of type packetType1 or of type packetType2.

I want to know if this is safe to do - 

packetType1 s1;
s1.data1 = 10;
s1.data2 = 22.22;


packetType2 s2;
s2.data1 = true; 

packet pack1; 
pack1.p1 = s1;

packet pack2;
pack2.p2 = s2;

message m1;
m1.type = 1;
m1.myPacket = pack1;

message m2;
m2.type = 2;
m2.myPacket = pack2;

eatPacket( &m1 );
eatPacket( &m2 );

void eatPacket( void *p )
{

    if( *(int*)p == 1)
    {
       message msg = *(message*)p
       cout << msg.myPacket.data1;
       cout << msg.myPacket.data2;
    }

    else if( *(int*)p == 2)
    {
       message msg = *(message*)p
       cout << msg.myPacket.data1;           
    }

}

Edit: ( For all those who are asking why I had used void* )

These messages are sent from one process to other using posix message queue and then decoded there. Problem is that even this message structure could be different. Only thing I am sure is that the variable int type will always be there to guide me. Other part of the message might change. So I had to make this function generic by making it accept a void * and then do decoding internally using the value provided by variable type.

Consider that someone makes a message like this now-

struct
{
    int type;

    float data;
    bool moreData;
    int evenMoreData;

} newMessage;

For this new message it was decided that value for variable type would always be 3.

So in my eat function I will just add another clause like this

if( *(int*)p == 3)
{
       newMessage msg = *(newMessage*)p
       cout << msg.data;
       cout << msg.moreData;
       cout << msg.evenMoreData;
}

Will it still be safe to do so ? I hope this makes sense now ?

timrau
  • 22,578
  • 4
  • 51
  • 64
Amit Tomar
  • 4,800
  • 6
  • 50
  • 83
  • 1
    Why take a `void*`? You can safely take a `message*`. – R. Martinho Fernandes Feb 09 '12 at 18:31
  • Also, why would you write something like `if (*(int*)p == 1)` unless you were deliberately trying to make the code difficult to understand. It may be safe (see http://stackoverflow.com/questions/281045/do-class-struct-members-always-get-created-in-memory-in-the-order-they-were-decl ), but why not make the code clearer (and shorter) by first casting to `message` or `message*` (though you should of course follow the advice of R. Martinho Fernandes above, and write the function as `eatPacket(message*)` instead of `eatPacket(void*)`). – Martin Wanvik Feb 09 '12 at 19:08
  • The if statements on `object.type` used here are exactly what C++ types, inheritance and method overloading can do for you. Am I wrong? (ref. my answer) – Johan Lundberg Feb 09 '12 at 19:40
  • @JohanLundberg and MartinWanvik and R.MartinhoFernandes - I hope this decision makes sense now ? (Please see the edit) – Amit Tomar Feb 10 '12 at 05:32

3 Answers3

3

It looks fine, but I'd rewrite eatPacket() like this:

void eatPacket(const message& msg)
{

    if(msg.type == 1)
    {
       cout << msg.myPacket.data1;
       cout << msg.myPacket.data2;
    }

    else if(msg.type == 2)
    {
       cout << msg.myPacket.data1;           
    }

}

There's really no need for the void* gymnastics that I can see. If you really need msg to be a pointer you can modify the above in a straightforward way (-> for ., etc).

Matt Phillips
  • 9,465
  • 8
  • 44
  • 75
  • @AmitTomar Sorry I didn't see your comment here until just now. Given the problem described in your edit, I'd say your solution is actually pretty clever--taking advantage of the fact that the address of a struct is identical to the address of its first member. The way I've always done this though is to send two separate messages, a 1-byte message telling the receiver process what the next message will be; in eat_packet there is a corresponding switch statement for proper message handling. Your way might be better! Have to worry about the next dev changing the order of the struct elts tho- – Matt Phillips Feb 13 '12 at 18:16
  • Thanks. I have put a restriction that the first element of this structure should always be int type. I know, not elegant, but will work as of now. Will look if I could use the 'sending 2 messages ' approach suggested by you. – Amit Tomar Feb 14 '12 at 12:41
1

What would I do?

void eatPacket( message* msg )
{
    if(NULL == msg) return;

    if( message->type == 1 )
    {
       cout << msg->myPacket.data1;
       cout << msg->myPacket.data2;
    }
    else if(message->type == 2 )
    {
       cout << msg->myPacket.data1;           
    }

}

Is it safe to do your way? I don't really know. What is message?

Dennis
  • 3,683
  • 1
  • 21
  • 43
1

I wouldn't do any of this. I would think it would be much cleaner to create an abstract BaseMessage class and then derive a class for the int payload and one for the bool payload. Then you could have a virtual GetData() method. When you pass in the pointer to the base class the correct virtual function will get invoked to return your data. A union is almost always a code smell that OO techniques can help with. A lot of how I would actually implement this depends on the variety of messages and how they eventually get used but hopefully you get the idea.

Tod
  • 8,192
  • 5
  • 52
  • 93