On Sat, 24 Apr 2004, MenTaLguY wrote:
On Sat, 2004-04-24 at 12:01, Carl Hetherington wrote:
Hi,
While doing some SPSelection OO-ification I have noticed that there seems to be some confusion in the code about whether desktop->selection is ever NULL in normal operation. Anyone got any clues?
I'm not sure. I don't think there was ever a defined policy for that -- everyone just programmed defensively with g_return_val_if_fail() and the like. Just in case.
The same confusion arises in other places where a SPSelection* is held. Personally, I'd favour the convention that all SPSelection* in structs are assumed to be non-NULL.
Generally speaking I would too -- this is another of my motivations for the C++ification, actually. It forces all these policy issues (formerly hidden by defensive programming) to the surface where we can deal with them.
OK. In my tree I've added a comment to the SPDesktop struct to the effect that the selection member will never "generally" be NULL. Perhaps we can make it "really" never NULL when SPDesktop is a class with a proper constructor.
I've then patched desktop-handles.cpp thus:
Index: desktop-handles.cpp =================================================================== RCS file: /cvsroot/inkscape/inkscape/src/desktop-handles.cpp,v retrieving revision 1.2 diff -u -r1.2 desktop-handles.cpp --- desktop-handles.cpp 16 Apr 2004 08:31:17 -0000 1.2 +++ desktop-handles.cpp 25 Apr 2004 14:02:54 -0000 @@ -28,8 +28,9 @@ SPSelection * sp_desktop_selection (SPDesktop * desktop) { - g_return_val_if_fail (desktop != NULL, NULL); - g_return_val_if_fail (SP_IS_DESKTOP (desktop), NULL); + g_assert(desktop != NULL); + g_assert(SP_IS_DESKTOP(desktop)); + g_assert(desktop->selection != NULL);
return desktop->selection; }
which means that the SP_DT_SELECTION macro will give an assertion failure if it doesn't return a non-NULL selection. I've then removed defensive checks from callers of SP_DT_SELECTION. Everything seems ok so far.
Do people think I should commit this stuff, so that people can find the bugs? Or would a more incremental approach be better?
Thanks
Carl