Scope Error
-
I just spent the best part of today finding a nasty little bug and thought I might "share" with you. We had some code that basically went
{ CPen apen; apen.CreatePen(...); dc->SelectObject(&apen); //Do lots of work with dc dc->SelectStockObject(BLACK_PEN); //Select apen out so it can clean up }Last version, the code was changed to something like{ if (option) { CPen apen; apen.CreatePen(...); dc->SelectObject(&apen); //Do lots of work with dc } else { CPen bpen; bpen.CreatePen(...); dc->SelectObject(&bpen); //Do lots of work with dc } dc->SelectStockObject(BLACK_PEN); //Select pens out so they can clean up }I am sure you can see the error... apen and bpen are now being destroyed before selected out. Resource leak. Running this code on Win2000/XP is fine because they clean up the mess. Running on 95,98,ME caused massive resource leaks (obviously!) The error is obvious when simplified as above but it was not the least bit obvious in the production code. It's always the simple things that bite you! Paul Hooper If you spend your whole life looking over your shoulder, they will get you from the front instead. -
I just spent the best part of today finding a nasty little bug and thought I might "share" with you. We had some code that basically went
{ CPen apen; apen.CreatePen(...); dc->SelectObject(&apen); //Do lots of work with dc dc->SelectStockObject(BLACK_PEN); //Select apen out so it can clean up }Last version, the code was changed to something like{ if (option) { CPen apen; apen.CreatePen(...); dc->SelectObject(&apen); //Do lots of work with dc } else { CPen bpen; bpen.CreatePen(...); dc->SelectObject(&bpen); //Do lots of work with dc } dc->SelectStockObject(BLACK_PEN); //Select pens out so they can clean up }I am sure you can see the error... apen and bpen are now being destroyed before selected out. Resource leak. Running this code on Win2000/XP is fine because they clean up the mess. Running on 95,98,ME caused massive resource leaks (obviously!) The error is obvious when simplified as above but it was not the least bit obvious in the production code. It's always the simple things that bite you! Paul Hooper If you spend your whole life looking over your shoulder, they will get you from the front instead.CPen* pOldPen = dc->SelectObject(&apen); //Do lots of work with dc dc->SelectObject(pOldPen); or you could just use SaveDC() and RestoreDC() so you do not have to keep track of what objects are selected. int nSavedDC = dc->SaveDC(); dc->SelectObject(&apen); //Do lots of work with dc dc->RestoreDC(nSavedDC); Trust in the code Luke. Yea right!
-
CPen* pOldPen = dc->SelectObject(&apen); //Do lots of work with dc dc->SelectObject(pOldPen); or you could just use SaveDC() and RestoreDC() so you do not have to keep track of what objects are selected. int nSavedDC = dc->SaveDC(); dc->SelectObject(&apen); //Do lots of work with dc dc->RestoreDC(nSavedDC); Trust in the code Luke. Yea right!
Oh! One more thing, do not do this:
if(..) { CPen apen; .... dc->SelectObject(&apen); //Do lots of work with dc }Unless you are going to restore the old pen before leaving the 'if' block, because (of course) apen goes out of scope as soon as you leave the block.
CPen apen; if(..) { .... dc->SelectObject(&apen); //Do lots of work with dc }Also it you must use CPen::CreatePen(), instead of the constructor, to create your pens then you must call CPen::DeleteObject() to destroy it or you will get memory leaks. Trust in the code Luke. Yea right!