Polymorphism is hard



  • Today I spent several hours trying to figure out why the hardware IO wasn't functioning as I expected under certain conditions in our 'inherited' project. In particular, several indicators on the simulated vehicle dashes were not flashing like they should. They would come on, if told to turn on, but they would not flash when told to flash. It was annoying, as I could put log messages into the code that told them to flash and verify that, yes, it was getting set to 'flash' instead of just 'on'.

    The hardware IO portion of our codebase uses a base 'EventHandler' class, with the various events that drive the hardware having their handlers derived off this class. Fairly standard stuff. The output specific section of the code kept a map of handlers and events, and whenever the event was received, called 'process' on the event handler and passed it the event data. This was working fine.

    However, during the 'update' cycle of the output manager, it also walked all the event handlers and called, 'handleFlashing'. This was supposed to do whatever voodoo needed to happen in order to make things flash. In this particular case, it counts the number of updates that have happened since a state change, and once they hit a certain level, invert the state (this is not time based, but frame based, which is another WTF in of it's own). For some reason the function and variable used to control how often the flip/flop occurs is called, 'hertz', implying that a value of '1' would be 1hz, meaning it'd change states ever second. This isn't the case, but that's a completely different WTF.

    No, the true fun comes in my 'modern' approach to calling this 'handleFlashing' method. Instead of a whole mess (say, 20-30, one for each event that drives outputs) of:

    ((IndicatorPanelEventHandler*)evntHandlers[EVT_INDICATOR_PANEL])->handleFlashing();

    I did...

    EventHandlerMapIter iter = evntHandlers.begin()
    while (iter != evntHandlers.end())
    {
    iter->second->handleFlashing();
    ++iter;
    }

    And it didn't work. Sadly, I didn't even think to check that the 'handleFlashing' was being called, as it surely was, right?

    Nope. For some reason, nothing in the base EventHandler class is virtual. NOTHING. I was calling an empty function that existed in the base class, and expecting it to do something. I have since rectified this oversight (who makes a base class and doesn't make any functions virtual?), and now lights should be flashing merrily away.

    Still.

    Makes one wonder.

    Also makes one want to drink. Thankfully there is a fridge full of beer at home, just waiting for me to get out of here. Which, sadly, won't be for a few more hours as I need to actually test this on hardware, and we're (software) only allowed to touch the dev hardware outside of core business hours (so before 8am, or after 6pm).


  • Discourse touched me in a no-no place

    @CodeNinja said:

    who makes a base class and doesn't make any functions virtual?

    But… but… but virtual functions are slow?!


  • Discourse touched me in a no-no place

    @CodeNinja said:

    For some reason, nothing in the base EventHandler class is virtual. NOTHING.

    Frankly, that's pretty funny (for an outside observer. I agree it's a total WTF to the person it bites.)


  • ♿ (Parody)

    My next step would be to look at what else that guy checked in. He sounds like the type who would create an enum for the various subclasses and use that in switch statements in order to cast to the right class and call the right methods.



  • @boomzilla said:

    My next step would be to look at what else that guy checked in. He sounds like the type who would create an enum for the various subclasses and use that in switch statements in order to cast to the right class and call the right methods.

    Sadly, it's old code. I'm not sure how old, exactly, but way before our company was put in charge of it. Probably created back in the 90s.

    Um... that switch / cast scheme does exist in here. I've seen it. Pretty much every event handler function takes an enumerated event type and a void pointer, which is then cast into the data structure for that event... or one that's close, at least. There are a couple of places where they cast the void pointer into a similar structure. Similar in that the first variable type is the same, and that's all they want, so they shove a bunch of possible events into one structure type and gods help you if someone changed something. 😢



  • @CodeNinja said:

    Um... that switch / cast scheme does exist in here. I've seen it. Pretty much every event handler function takes an enumerated event type and a void pointer, which is then cast into the data structure for that event... or one that's close, at least.

    Reminds me of btRigidBody::upcast().

    static const btRigidBody* upcast(const btCollisionObject* colObj)
           {
                  if (colObj->getInternalType()&btCollisionObject::CO_RIGID_BODY)
                            return (const btRigidBody*)colObj;
                  return 0;
           }
    

    You used to be able to dynamic_cast from btCollisionObject to btRigidBody in earlier versions, but now you get a compiler error if you try. In their defense, they're writing a real-time physics library, so vtable lookups might be too expensive to their taste.


Log in to reply