This one wasn’t a bug that landed immediately from my change, but happened when a subsequent code branch was integrated. I’m going to claim this as my bug as it could have been prevented (or at least made more obvious) with some extra though.<\/p>\n
We have a virtual function, something like<\/p>\n
class Base {\r\npublic:\r\n\u00a0 virtual void Foo(int some_param) \u00a0{ \u00a0}\r\n};<\/pre>\nWhich I modified a few weeks back by adding an extra parameter, so foo() became<\/p>\n
virtual void foo(int some_param, WhatsitType some_other_param) { }<\/pre>\nand I added the same parameter to every one of the classes that inherited from class Base. There were quite a few.<\/p>\n
Sometime later, \u00a0code was integrated from another branch where the old prototype was still in use, including something like this:<\/p>\n
class NewInheritor : public Base {\r\npublic:\r\n virtual void Foo(int some_param) { \/\/ oh noes - it's the old prototype!\r\n DoAmazingThings();\r\n }\r\n};<\/pre>\nThe new code compiled without warning, but at runtime, NewInheritor::Foo()<\/strong> wasn’t being called, so the amazing things were failing to be done. Debugging this was somewhat inefficient — the process for calling Foo()<\/strong> methods includes a number of steps, and I failed to notice the missing parameter for quite a while.<\/p>\nThe fix:<\/h3>\n
Update NewInheritor::Foo()<\/strong> to match the parent class.<\/p>\nPreventing these in future:<\/h3>\n
Remember that virtual inheritance is a horribly fragile beast.<\/p>\n
One thing that may have worked is to make Foo()<\/strong> pure (compile error) — or undefined (link error) — in the base class (i.e. void\u00a0Foo() = 0;<\/strong>). This isn’t a great match in this case as inheriting classes don’t need to have their own implementation of Foo()<\/strong> to be valid, and requiring it makes for some additional tedious boilerplate code in every inheritor (of which there are many).<\/p>\nThe override<\/strong> keyword will help detect this at compile time, but it relies on the implementer of NewInheritor<\/strong> to have used it :\\ The action here is to recommend that people use override<\/strong> on every virtual function override.<\/p>\nIn this case, what I do know about this code is that it is never valid to call Foo()<\/strong> on a class that doesn’t have its own implementation. We can put an assert()<\/strong> in Base::Foo()<\/strong> and let the writers of inheriting types (or me when next I get to debug one of these) have a more obvious clue about what’s happening. Adequately functional.<\/p>\nmemcpy()<\/strong> into an uninitialized pointer variable<\/h2>\nThe problem:<\/h3>\n
I had changed the type of a variable from being a static array to being a pointer, to match a change I’d made to a particular function. What I hadn’t paid attention to was that this array was being used as the destination address for a call to memcpy()<\/strong>. An uninitialized pointer on the stack is not a good place to try to copy things.<\/p>\nThe call to memcpy()<\/strong> did not cause the program to crash. The program did crash shortly thereafter, though, which distracted me for a short while from the cause.<\/p>\nThe fix:<\/h3>\n
Explicitly allocate some space for the copy.<\/p>\n
Preventing in the future:<\/h3>\n
The pointer on the stack had not been set. A null pointer would have caused the crash to happen at the call to memcpy()<\/strong>, making the cause more obvious.<\/p>\nBad loop counting<\/h2>\nThe problem:<\/h3>\nint max = m_Max;\r\nint i = 0;\r\nfor( ; i < max; ++max )\r\n\u00a0 if( a[i] == bar )\r\n break;<\/pre>\n++max? Yeesh.<\/p>\n
m_Max is initialized to zero, which means that this was a no-op rather than an infinite loop.<\/p>\n
The fix:<\/h3>\n
Increment the counter, not the upper bound.<\/p>\n
Preventing in the future:<\/h3>\n
Don’t be a goose.<\/p>\n
Failing to store updated state<\/h2>\nThe problem:<\/h3>\n
Immediately following the code above, in the case where we failed to find the desired bar<\/strong> in the array, increase the size of the array. And then fail to store the new max size into m_Max.<\/p>\nI even got this wrong twice: my first attempt was to replace a use of\u00a0max+1<\/strong> with ++max<\/strong>.<\/p>\nThe fix:<\/h3>\n
Write the necessary state back to the object: ++m_Max.<\/strong><\/p>\nPreventing in the future:<\/h3>\n
Pay attention to where the state should go. Don’t be a goose.<\/p>\n","protected":false},"excerpt":{"rendered":"
Changed virtual function prototype results in correct function not being called This one wasn’t a bug that landed immediately from my change, but happened when a subsequent code branch was integrated. I’m going to claim this as my bug as it could have been prevented (or at least made more obvious) with some extra though. … Continue reading “Bugs of the day, Tuesday 2013-03-19”<\/span><\/a><\/p>\n","protected":false},"author":2,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":[],"categories":[26],"tags":[],"_links":{"self":[{"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=\/wp\/v2\/posts\/1114"}],"collection":[{"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=\/wp\/v2\/users\/2"}],"replies":[{"embeddable":true,"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=1114"}],"version-history":[{"count":6,"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=\/wp\/v2\/posts\/1114\/revisions"}],"predecessor-version":[{"id":1121,"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=\/wp\/v2\/posts\/1114\/revisions\/1121"}],"wp:attachment":[{"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=1114"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=1114"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/brnz.org\/hbr\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=1114"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}