Hi Atul,

You were faster than me in replying ;-). I totally agree with you! Here is what I wanted to post right before I saw your reply. It would have been a pity not to post what took me like half an hour to type on my way back home ;-)

Hi Atul! Thanks for the post!

I’ve never thought about using static_cast instead of dynamic_cast in castFrom() and the idea seemed reasonable to me at first, but on a second thought I got some concerns…


static inline Bye & castFrom (Packet & packet);
static inline const Bye & castFrom (const Packet & packet);


You see that the functions are static and are supposed to be used like this:


switch (packet.payloadDescriptor()) {
case Gnutella::Packets::ByeDescriptor:
{
Ping &ping = Bye::castFrom (packet);
// ...
}
break;
//..
}


The assumption here is that the packet was parsed correctly and has the correct dynamic type. Now suppose there existed a bug, which would have caused a packet of actual dynamic type UnknowPacket to be created instead of a Bye. Furthermore, if we used static_cast in castFrom(), then the cast would have succeeded and the application would have most likely crashed pretty badly a little later, without giving us any hint what went wrong, when some Bye specific function gets invoked. If dynamic_cast is used, then a bad_cast exception will be thrown and we will know what exactly happened.

Maybe in another case someone would expect a Query packet and would want to do a cast without verifying the payloadDescriptor(). I’d prefer the application to stop due to an an exception being thrown or a failed Q_ASSERT instead of as a consequence of an incorrect assumption with less or no trace of what went wrong.

You said that the safety offered by dynamic_cast is good and I would like to get this bit of safety, as it would help us spend less time on hunting hard to find bugs and will save us the trouble of writing explicit Q_ASSERTS.

Peter

…

atul wrote:I think I was a little sleepy when I wrote that….

Now as I think I am wide awake ;-) , as it can not be guaranteed here by design ( as in CRTP ), dynamic_cast seems to be the correct choice…

( like in CRTP you cannot guarantee that packet would always be Bye, dynamic_cast is correct — as it will throw an exception if the cast is wrong )

I will probe further to see why we need castForm etc…

– cheers atul

atul wrote:

Hi all

While going thru Bye.h

inline Bye & Bye::castFrom (Packet & packet)
{ return dynamic_cast (packet); }
I think static_cast would be a better choice here…

The reason is that as the compiler can see the line
class Bye : public Packet

the static cast should be safe… ( We don’t need the heavy duty dynamic_cast here )

I mean here it may not matter much but if some cycles are saved the better…. and in other places it might make some difference.

Also dynamic_cast implies RTTI must be used and generated… here anway it is as Packet::copy is the virtual constructor stuff… but other places it could matter ;-)

I accept that safety offered by dynamic_cast is good and yes “Early optimization is the root of all evil” but still

here are my two cents … ;-)