[haiku-commits] Re: haiku: hrev47869 - src/apps/bootmanager

  • From: Stephan Aßmus <superstippi@xxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Thu, 18 Sep 2014 11:03:01 +0200

Hi,

Am 18.09.2014 10:36, schrieb Axel Dörfler:
Am 18.09.2014 05:53, schrieb jessica.l.hamilton@xxxxxxxxx:
+    fIcon = new BBitmap(BRect(0, 0, B_LARGE_ICON - 1, B_LARGE_ICON - 1),
+        B_RGBA32);

The bitmap might be invalid if the allocation failed.

+    if (device.GetIcon(fIcon, B_LARGE_ICON) != B_OK)
+        memset(fIcon->Bits(), 0, fIcon->BitsLength());

Why present a black box in this case? I would find it much nicer to do
it this way:

Oh, is it a black box? Have you tested? The bitmap would need to be rendered with B_OP_OVER already, to take account for transparency in icons, and then it shouldn't be a black box, but completely transparent.

if (!fIcon.IsValid()
     || device.GetIcon(fIcon, B_LARGE_ICON) != B_OK) {
     delete fIcon;
     fIcon = NULL;
}

BBitmap allocation can acually fail in two places, but you advertize fixing only one. Intentionally?

But I wonder if it is worth it (since I will bet important body parts that almost none of the GUI creation code in our repo checks every allocation). device.GetIcon() will also check that the BBitmap is valid, BTW, and so will Draw(). Maybe memset() only when GetIcon() failed and the BBitmap itself is valid, otherwhise one would try to write to a buffer that isn't there. But I don't know if it makes sense to go further. The difference being that allocation failure inside the app should just crash it, while allocation failure on app_server side could be due to fragmentation and be handled gracefully.

Best regards,
-Stephan

Other related posts: