[haiku-commits] Re: haiku: hrev47360 - src/add-ons/input_server/filters/shortcut_catcher

  • From: Stephan Aßmus <superstippi@xxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Fri, 13 Jun 2014 09:29:44 +0200

Hi John,

Am 13.06.2014 01:27, schrieb jscipione@xxxxxxxxx:
@@ -323,16 +324,17 @@ KeyCommandMap::MessageReceived(BMessage* msg)

-//! Deletes an HKS-filled BList and its contents.
+// deletes the BList and its contents
-KeyCommandMap::_DeleteHKSList(BList* l)
+KeyCommandMap::_DeleteHKSList(BList* list)
-       if (l != NULL) {
-               int num = l->CountItems();
-               for (int i = 0; i < num; i++)
-                       delete ((hks*) l->ItemAt(i));
-               delete l;
-       }
+       if (list == NULL)
+               return;
+       while (list->ItemAt(0) != NULL)
+               delete list->ItemAt(0);
+       delete list;

I know you reverted this commit. As you probably know by now, the previous version of the code was just fine. I just want to make you aware of other problems with the code that you probably /meant/ to write:

+       while (list->ItemAt(0) != NULL)
+               delete list->RemoveItem((int32)0);

This would have had the problem that it stops at NULL items in the list, leaking any non-NULL items that follow it. Regardless of whether the list is even supposed to potentially contain NULL items or not, I would still avoid code that has such potential for error.

The second problem is that it actually performs much worse. RemoveItem() triggers reallocations, which in the context are completely unnecessary. The original form of the loop had none of these problems, just for when you encounter it in the future.

The third problem with your commit is that you obviously did not test your new code, or else you would have seen it crash with a double delete or halt in an infinite loop. To sum up: Please no misguided cleanups that break things. When you do cleanup commits, you really need to make extra sure not to break stuff, especially when you rewrite code. Even when you just rename a variable, you could do non-obvious stuff like change how some local variable is shadowing another. So in principle, you have to always test well.

Best regards,

Other related posts: