Exception handling?
in [MFC]
|
Prev: Deriving from a class derived from a CDialog
Next: How to show underlined menu shortcut letters by default for a contextmenu ?
From: Goran on 22 Jun 2010 10:18 On Jun 22, 3:45 pm, "RB" <NoMail(a)NoSpam> wrote: (First off, I rather poorly worded the "user" exception part, hopefully you understood (seems so).) > I've learned so far (from the Debugger) that I don't seem > to need to catch any exceptions other than my own. Not even that. Why do you think that? (That is, show an example, let's discuss it). More seriously, go back to my advice: when you think that you should catch, first find where it's going to end if you __don't__ catch it. Doing that is an eye opener, it really is. For example, if you are in the middle of a message loop in your program (e.g. you are handling some message, menu command, dialog control notification...), you should almost never catch __anything__. Just let exception fly. It ends up in ProcessWndProcException and gets reported (using ReportError()). So as long as you made your exceptions "reportable" (Joe speaks about that, kinda-sorta), you are golden. Of course, you have to clean up any resources, or internal state, properly (that is, you need to make your code exception-safe), but that's another story. What Joe says about exceptions in general is also right on the money (the "Never be afraid to create your own classes of exceptions when needed..." paragraph). __THAT'S__ how things are done well. Another related observation, you'll find that on the internet if you look, too... Normally, your exception classes hierarchy will typically be wide, but +/- shallow (and indeed, that's what happens to Joe in his "syntax" story). Goran.
From: RB on 22 Jun 2010 11:17 Hi Goran, Got a little bit of logic only stuff here for you that I tried on my work break, but I won't be able to fully clean up my code till I get home tonight. > (First off, I rather poorly worded the "user" exception part, > hopefully you understood (seems so).) Ugh well, somethings I understood, others I am still tearing down my ignorance piece by piece >> I've learned so far (from the Debugger) that I don't seem >> to need to catch any exceptions other than my own. > Not even that. Why do you think that? (That is, show an example, > let's discuss it). Ugh, well if I don't code any exception handlers at all in my code, and I step thru the serial read (on a file I have made sure is corrupt), it takes me to and end of file handler which does all of what I previously said. So I would think that at least the reading past end of file looking for stuff ( resulting from grabbing "prewritten length" bytes where they are supposed to be, but were not ) would be covered without me coding any exception handler. Your question makes me feel I still have not conceptualized this area enough yet. Additionally I have this logic going so far. // in the read loop of MyDoc class serialize else { try { ar >> FID_Read;; // DWORD FID_Read; if (FID_Read != FileID) // const DWORD FileID; { // FileID mismatch throw new CWrongFileIDException(); } ar >> VerData.Ver >> VerData.CpyRt >> VerData.Corp; ar.SerializeClass(RUNTIME_CLASS(CMapStringToString)); ExpMap1.Serialize(ar); } catch(CWrongFileIDException* e) { // Do whatever I may need here // so far nothing that I can tell, in fact it seems I could have just // eliminated my try and catch handlers and just called the // AfxThrowArchiveException in the if loop above ? e->Delete( ); AfxThrowArchiveException(CArchiveException::badIndex, NULL ); //Invalid file format // The above takes me to the exact same cleanup code that I get when // I read in a corrupt file (with NO exception code at all ) and mfc handlers it. // And leaves me with an untitled filename eliminating the change if inadvertant // save overwrite. } }
From: Joseph M. Newcomer on 22 Jun 2010 13:03 See below... On Tue, 22 Jun 2010 01:15:42 -0700 (PDT), Goran <goran.pusic(a)gmail.com> wrote: >On Jun 21, 10:33�pm, Joseph M. Newcomer <newco...(a)flounder.com> wrote: >> I should also add that the behavior of MFC if an exception is thrown from inside >> Serialize() is not documented, and therefore, it does whatever it does. �The next release >> might do something completely different! > >Whoa, not true! It's documented in help for >CDocument::ReportSaveLoadException. > >CDocument::ReportSaveLoadException is not perfect (it actually only >handles CFile/Archive exceptions, and for the rest it pops (poops, >rather) up silly "generic" text ("Failed to open document" I think), >so an override of that function might be deemed necessary. **** OK, I had not looked there. I only looked at CObject::Serialize, which SHOULD have had a cross-reference to CDocument::ReportSaveLoadException, and didn't. joe **** > >It's not undocumented. > >Goran. Joseph M. Newcomer [MVP] email: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm
From: Joseph M. Newcomer on 22 Jun 2010 13:12 See below... On Tue, 22 Jun 2010 01:10:21 -0700 (PDT), Goran <goran.pusic(a)gmail.com> wrote: >On Jun 21, 6:13�pm, "RB" <NoMail(a)NoSpam> wrote: >> In my below foobar first attempt to try exception handling I don't understand >> the following aspects: �(my code at bottom after these framework pastes) >> >> 1. I must not be doing something correct because even though both >> my CATCH and AND_CATCH call stuff on return that appears to be >> deleting the exception and taking care of various cleanup. > >I would also recommend that you forget TRY, CATCH and the rest of MFC >macros. They are a sad leftover from the time MS C++ compiler didn't >implement exceptions (but MFC people wanted to have them). > >MFC has that nasty design flaw with exception handling that it uses >exception __pointers__ (I guess also due to said lack of exceptions in >compiler in early days). This is not normal in C++ code, and a >horrific error. Consequence of these pointers is that design of >ownership of said pointers is complicated (well, not much, but still): >when you catch an MFC exception, and you don't want to re-throw it, >you must call Delete() on it (__NOT__ e.g. delete pException;) **** Note that the Delete() method has the additional (mis)feature that if it throws a pointer to a statically-allocated object that the Delete() is smart enough to not try to delete it; however, the delete operator will usually result in an assertion failure in the allocator. Yes, the whole MFC mechanism had to be backward-compatible with the old macros, which I believe once did setjmp/longjmp and therefore needed pointers, so when real C++ exceptions were added, they had to fit them into a world where pointers were used. Unfortunately, none of the documentation is correct and does not explicitly state that pointers are being used. **** > >I use C++ try/catch and a macro DEL_ON_EXIT, that I put on top of >every catch where I want to handle the exception, e.g. like so: > >try >{ >workworkwork(); >} >catch(CException* p) >{ > DEL_ON_EXIT(p); > p->ReportError(); >} > >DEL_ON_EXIT calls p->Delete() on block exit. > >Warning: I'll tear apart code below, it's not good at all. ;-) > > >> void CFileHandlingDoc::Serialize(CArchive& ar) >> { >> �if (ar.IsStoring()) � � � >> � �{ >> � � � ar << FileID; � �// DWORD >> � � �ar << VerData.Ver << VerData.CpyRt << VerData.Corp; � >> � � �ar.SerializeClass(RUNTIME_CLASS(CMapStringToString)); >> � � �ExpMap1.Serialize(ar); � � >> � �} � � � � � � � � � � � � � � � � � � � � >> �else � � � � � � � >> � �{ >> � � �CString E; >> � � �TCHAR szErrMsg[255]; >> � � �TRY >> � � � { >> � � � � ar >> FileID; � � � � � � � � >> � � � � if (FileID != 0x1234ABCD) >> � � � � � { // FileID mismatch � � � � � � � � � � � � >> � � � � � � AfxThrowUserException( ); > >This is bad. CUserException is a special exception type. You use it >like so: first, you inform the user what went wrong, then you throw it >and leave it to MFC to "handle". MFC actually ignores it, assuming >that user was already informed about what went wrong. > >So in this case, you first inform the user what went wrongm then >throw. What happens in this particular case is that it goes to >document's ReportSaveLoadException and gets ignored there. > >So... Given how CUserException is specified, inform the user about >what went before throwing. That's rule 0 of CUserException. Rule 1 is: >avoid using it. Given that you need to inform the user what's >happening, it's better to throw an exception that contains whatever >info about the error you want (including message to the user) catch it >and report it higher up the stack (most likely, that means letting it >be handled by MFC somewhere). There's a catch WRT >ReportSaveLoadException and that idea, though, so for now, just inform >the user and throw "user" exception. > >> � � � � � } >> � � � � ar >> VerData.Ver >> VerData.CpyRt >> VerData.Corp; >> � � � � ar.SerializeClass(RUNTIME_CLASS(CMapStringToString)); // WriteClass CmStS's data >> � � � � ExpMap1.Serialize(ar); � �// CMapStringToString's keys and values >> >> � � � } > >This is bad, and it shows massive confusion WRT exceptions. In 99% of >cases, if you throw and catch same exception type in one function, >you're doing it wrong. > >In fact, a piece of advice for a beginner: in 99% of cases, if you >think that you need to write a try/catch, you're doing it wrong. What >you should do instead is ask yourself "if I throw here, where is >exception caught?" Find the answer to that, and you'll find that you >don't need a try/catch. (Exception to that rule: various "creation" >functions of MFC that you can override, e.g. PreCreateWindow, >OnCreate, OnInitDialog, stuff like that especially e.g. OnCreate >should not allow that exception escapes from them. > >> � � �CATCH( CUserException, e ) >> � � � { >> � � � � E = _T("BAD FileID, RB from inside CATCH\n"); >> � � � � AfxMessageBox( E ); >> � � � � return; >> � � � } >> � � �AND_CATCH( CException, e ) >> � � � { >> � � � � // For other exception types, notify user here. >> � � � � e->GetErrorMessage(szErrMsg, 255); >> � � � � E = _T("RB from inside AND_CATCH\n"); >> � � � � E += szErrMsg; >> � � � � AfxMessageBox( E ); >> � � � � return; � >> � � � } >> � � �END_CATCH >> � � � � // No exception thrown. >> � �} >> >> } � � � � � � � � � � > >In your case, there should not be ANY try/catch statements in the >whole of Serialize. What you should do, instead, is let exceptions go >through. They will end up in ReportSaveLoadException and be handled >there by MFC. > >Goran. > >P.S. Don't forget: you are not allowed to write try/catch >statements ;-). If you do, you have 99% chance that you're doing it >wrong. Good C++ code of today has RIDICULOUSLY small number of try/ >catch-es in ti. **** I'm not sure this is a good piece of advice. I use try/catch a lot; it is essentially a "non-local GOTO", a structured way of aborting execution and returning to a known place, while still guaranteeing that all intermediate destructors for stack variables are called. It is particularly useful in writing tasks like recursive-descent parsers (particularly if you just want to stop without trying to do error recovery, which is always hard) and terminating threads while still guaranteeing that you return from the top-level thread function. It is clean and well-structured way of aborting a partially-completed operation. An it is the only way to report errors from operations like 'new'. Also, look at the number of exceptions that can be thrown by std:: or boost::. joe Joseph M. Newcomer [MVP] email: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm
From: Joseph M. Newcomer on 22 Jun 2010 13:29
See below.... On Tue, 22 Jun 2010 09:45:51 -0400, "RB" <NoMail(a)NoSpam> wrote: > >> This is bad. CUserException is a special exception type. >>.......... >>........, >> it's better to throw an exception that contains whatever info >> about the error you want (including message to the user) catch it >> and report it higher up the stack (most likely, that means letting it >> be handled by MFC somewhere). There's a catch WRT >> ReportSaveLoadException and that idea, though, so for now, >> just inform the user and throw "user" exception. > >Ok, ( Joe previously told me I had missed the concept by >trying to use another Exception for my specific scenario) so >I am understanding that. And even though I not really ready >to ask any new questions currently, I did want to respond >and let you guys know that you have me on the right track, and >I am working on cleaning up my methods while I also learn loads >about what is going on and what I need to do in my handler. > I've learned so far (from the Debugger) that I don't seem >to need to catch any exceptions other than my own. IOW >MFC is going to catch a slew of them on it's on and if I >interfere with a catch on my end, then I had better know >how to forward the throw back into the mfc handler or I >am going to lose all of the highly important code thereof. **** But MFC doesn't always handle them "gracefully". For example, in the inner loop of the MFC command dispatcher, if it gets an excpeption it pops up a completely useless MessageBox which helps neither the end user nor tech support in the slightest. **** >Namely like this Note all my comments are // RB xxxx > >BOOL CDocument::OnOpenDocument(LPCTSTR lpszPathName) >{ > ....... > ....... > CATCH_ALL(e) > { > ReleaseFile(pFile, TRUE); > DeleteContents(); // remove failed contents > TRY > { > // RB this (as you already said ) takes me to code > // that sorts the exception and reports a msg to > // user. > ReportSaveLoadException(lpszPathName, e, > FALSE, AFX_IDP_FAILED_TO_OPEN_DOC); > ........ >// RB, then after returning back CArchive code >// shuts down the archive and returns back to > >CDocument* CSingleDocTemplate::OpenDocumentFile( > LPCTSTR lpszPathName, BOOL bMakeVisible) > { > ...... > ...... > // RB, and comes to this "all important" code which will set the > // current document (on return to user ) to "untitled" so that > // an inadvertant save does not overwrite any other filename. > // I would not get this if I interrupted this exception with my > // catch and did not forward it along > > if (!pDocument->OnOpenDocument(lpszPathName)) > { > // user has been alerted to what failed in OnOpenDocument > TRACE0("CDocument::OnOpenDocument returned FALSE.\n"); > if (bCreated) > { > pFrame->DestroyWindow(); // will destroy document > } > else if (!pDocument->IsModified()) > { > // original document is untouched > pDocument->SetModifiedFlag(bWasModified); > } > else > { > // we corrupted the original document > SetDefaultTitle(pDocument); >// RB the above is what I was looking for and and now I have >// figure out how to call this and any other needed code from any >// scenario exception throw of my own that does not finish opening >// the document. >........... >........... >// RB or otherwise I am going to get this, which >// is not good and will effect and inadverntant >// overwrite > pDocument->SetPathName(lpszPathName); >............ > return pDocument; >} *** But the point here is that it *has* called the virtual method ReportSaveLoadException. and it return FALSE from OnOpenDocument. For some reason, you omitted that critical code from this post, although it is present in the actual CDocument::OnOpenDocument method source code! Because this is a virtual method, you could override it and if calling the parent returned NULL, you could do something useful such as setting the filename to empty. I think this code actually is incorrect, but you can create your own subclass and do whatever you want. joe Joseph M. Newcomer [MVP] email: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm |