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: RB on 21 Jun 2010 12:13 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. // abbreviated Step thru pastes //--------------------------------------- void AFXAPI AfxTryCleanup( ) { ...... ..... // delete current exception ASSERT(pLinkTop != NULL); if (pLinkTop->m_pException != NULL) pLinkTop->m_pException->Delete( ); ..... ..... } // and later CString::~CString() // free any attached data { ....etc etc } //-------------------------------------- * * when the exception returns back to document caller I don't see this * * getting executed, if (!pDocument->OnNewDocument()) { // user has been alerted to what failed in OnNewDocument TRACE0("CDocument::OnNewDocument returned FALSE.\n"); if (bCreated) pFrame->DestroyWindow(); // will destroy document return NULL; } ..... ....... // Instead I see this down the road, CDocument* CWinApp::OpenDocumentFile(LPCTSTR lpszFileName) { ASSERT(m_pDocManager != NULL); //* *And lpszPathName still = TheFileNameItriedToOpen ? * * return m_pDocManager->OpenDocumentFile(lpszFileName); } //* * So if I save my document after code turns app back over to me I overwrite the file that I could not open ? So there must be more " I " have to do, or something different if I have it all wrong out of the shute. 2. Also from the docs I have read I got the impression that by putting THROW_LAST( ); at the end of my CATCH( CUserException, e ) that it would forward the exception to my AND_CATCH( CException, e ) but experimentation shows that does not happen, so I missed something there. But anyhow both exceptions seem to pass to clean up code on return so question 2 is only a curiousity. Question 1 is my main concern. === foobar attempt below ==== 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( ); } ar >> VerData.Ver >> VerData.CpyRt >> VerData.Corp; ar.SerializeClass(RUNTIME_CLASS(CMapStringToString)); // WriteClass CmStS's data ExpMap1.Serialize(ar); // CMapStringToString's keys and values } 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. } }
From: Joseph M. Newcomer on 21 Jun 2010 13:16 See below... On Mon, 21 Jun 2010 12:13:13 -0400, "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. **** You should avoid these obsolete macros. They were created before the C++ compiler correctly implemented (or implemented at all) the C++ exception syntax. **** > >// abbreviated Step thru pastes >//--------------------------------------- >void AFXAPI AfxTryCleanup( ) >{ > ...... > ..... > // delete current exception > ASSERT(pLinkTop != NULL); > if (pLinkTop->m_pException != NULL) > pLinkTop->m_pException->Delete( ); > ..... > ..... >} >// and later >CString::~CString() > // free any attached data >{ > ....etc etc >} >//-------------------------------------- >* * when the exception returns back to document caller I don't see this >* * getting executed, > > if (!pDocument->OnNewDocument()) > { > // user has been alerted to what failed in OnNewDocument > TRACE0("CDocument::OnNewDocument returned FALSE.\n"); > if (bCreated) > pFrame->DestroyWindow(); // will destroy document > return NULL; > } > ..... > ....... >// Instead I see this down the road, > >CDocument* CWinApp::OpenDocumentFile(LPCTSTR lpszFileName) >{ > ASSERT(m_pDocManager != NULL); > //* *And lpszPathName still = TheFileNameItriedToOpen ? * * > return m_pDocManager->OpenDocumentFile(lpszFileName); >} >//* * >So if I save my document after code turns app back over to me I overwrite >the file that I could not open ? So there must be more " I " have to do, or something >different if I have it all wrong out of the shute. > >2. Also from the docs I have read I got the impression that by putting THROW_LAST( ); > at the end of my CATCH( CUserException, e ) that it would forward the exception > to my AND_CATCH( CException, e ) but experimentation shows that does not > happen, so I missed something there. But anyhow both exceptions seem to pass > to clean up code on return so question 2 is only a curiousity. Question 1 is my > main concern. > >=== foobar attempt below ==== > >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 **** Use try (lower case) > { > ar >> FileID; > if (FileID != 0x1234ABCD) **** Why is this a hardwired literal? Why did you not do const DWORD CURRENT_FILEID = 0x1234ABCD; ? **** > { // FileID mismatch > AfxThrowUserException( ); **** I would have done throw new CWrongFileIDException(FileID) where I would have defined a specific exception that did exactly what I wanted: class CWrongFileIDException : public CException { public: DWORD GetFileID() { return fid; } CWrongFileIDException(DWORD fileid) { fid = fileid; } protected: DWORD fid; }; You should not "hijack" some other exception to mean other than what precisely happened! **** > } > ar >> VerData.Ver >> VerData.CpyRt >> VerData.Corp; > ar.SerializeClass(RUNTIME_CLASS(CMapStringToString)); // WriteClass CmStS's data > ExpMap1.Serialize(ar); // CMapStringToString's keys and values > > } > CATCH( CUserException, e ) **** Take a look at CUserException (by the way, you should not use built-in exceptions like this) Instead, you should write your own exception (like above) then to throw it you would do throw new CWrongFileIDException(FileId); Get rid of CATCH and write catch(CUserException* e) or, if you define a CRBException, use catch(CWrongFileIDException * e) Note that the documentation of AfxThrowUserException does NOT state what kind of exception it throws, and it says that it throws an exception to "stop an end-user operation". But if you read the actual code, it CLEARLY throws a CUserException*. For most exceptions, the documentation is at best confusing, and it is usually merely erroneous. In this case, it is nonexistent. So why did you think it throws a CUserException when you have no documentation to rely on? If it doesn't tell you what it does, you don't know. So in that case, "trust the source, Luke". I read the documentation, and at least in the version I have, it says absolutely NOTHING about what is thrown. And there is no reason to suspect that Microsoft would not change this in the future. So why are you relying on an effectively completely undocumented function? **** > { > E = _T("BAD FileID, RB from inside CATCH\n"); > AfxMessageBox( E ); > return; **** Query: why are you putting an English language message in SOURCE code? That's what STRINGTABLEs are for! Query: Why did you declare the variable E so far from where it is used? It should be written, if you want to keep the really bad idea of English strings, as CString E = _T("..."); that is, you should never declare a variable in a scope wider than it needs to exist! And for that matter, why did you have to declare a CString for AfxMessageBox? If you want to keep the Really Bad Idea, do AfxMessageBox(_T("...")); don't introduce variables you don't need, and NEVER declare them with a scope larger than you need! Note that AfxMessageBox allows you to give the UINT of a STRINGTABLE ID, so if you are using a literal string, you would not say anything more than AfxMessageBox(IDS_BAD_FILE_ID); You should also do e->Delete(); to handle the case where the exception was thrown with 'new' **** > } > AND_CATCH( CException, e ) **** Due to terminal brain damage on the documentation team, they keep erroneously claiming, for example, that some operation "Throws a CFileException" or something like that, when the TRUTH is that they throw a CFileException*. This is slovenly documentation, and I've been complaining about it for years. As far as I can tell, NO exception in MFC is EVER other than a derived class of CException*, not CException. So the documentation lies in its teeth. Get rid of tne AND_CATCH, and replace it with catch(CFileException * e); **** > { > // For other exception types, notify user here. > e->GetErrorMessage(szErrMsg, 255); **** szErrMsg should not have a scope outside these braces. Why is it declared anywhere else? Same applies to E. Why did you not write TCHAR szErrMsg[255]; e->GetErrorMessage(szErrMsg, _countof(szErrMsg)); ? If you are going to use a size, either use _countof EVERYWHERE to reference it (if it is a compile-time constant array) or a symbolic length name **** > E = _T("RB from inside AND_CATCH\n"); > E += szErrMsg; > AfxMessageBox( E ); **** Don't forget to do e->Delete(); **** > return; > } > END_CATCH **** There is no need for END_CATCH. These macros are obsolete and should not be used in modern programming. They are leftovers from the 16-bit C++ compiler of 1993 or so, and were effectively obsolete by (if I remember correctly) VS 4.2. Certainly by VS6. joe **** > // No exception thrown. > } >} > Joseph M. Newcomer [MVP] email: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm
From: RB on 21 Jun 2010 15:21 > You should avoid these obsolete macros. They were created before the > C++ compiler correctly implemented (or implemented at all) the C++ > exception syntax. > **** > Use try (lower case) >> { >> ar >> FileID; >> if (FileID != 0x1234ABCD) > **** Ok, noted, you gave me so much to work with I will have to get back with you on most of this Actually what I am most concerned with right now is learning all of what you just gave me. I stepped thru my doc return code again and it appears that is not currently doing what I had hoped. I realize if I click SAVE it is going to save whatever new document data in currently in the members. What I was hoping for was that the exception code would void out the current lpszFileName doc variable so that at least when I click save it would prompt me for a filename instead of just overwriting what is in the lpszFileName. > Why is this a hardwired literal? Why did you not do > const DWORD CURRENT_FILEID = 0x1234ABCD; > ? > **** Ugh maybe because I am a dummy and have not learned any better? >> { // FileID mismatch >> AfxThrowUserException( ); > **** > I would have done > throw new CWrongFileIDException(FileID) > where I would have defined a specific exception that did exactly what > I wanted > class CWrongFileIDException : public CException { > public: > DWORD GetFileID() { return fid; } > CWrongFileIDException(DWORD fileid) { fid = fileid; } > protected: > DWORD fid; > }; > > You should not "hijack" some other exception to mean other than what > precisely happened! > **** Ok give me some time to work with this. > **** > Query: why are you putting an English language message in SOURCE code? > That's what STRINGTABLEs are for! This was only just for "me" to see if / when it was actually called. > Query: Why did you declare the variable E so far from where it is used? > It should be written, if you want to keep the really bad idea of English strings, > CString E = _T("..."); > that is, you should never declare a variable in a scope wider than it needs to exist! The stuff about where to declare the vars I really just have not gotten that far yet, but appreciate it and will try to incorporate it into my current attempt to learn exceptions. > And for that matter, why did you have to declare a CString for AfxMessageBox? Well for the one I did not have to, but I got into the CString originally in order to copy the strings together e->GetErrorMessage(szErrMsg, 255); E = _T("RB from inside AND_CATCH\n"); E += szErrMsg; // Again the English is just for me to "see" when it was called. > ..... > You should also do > e->Delete(); > to handle the case where the exception was thrown with 'new' > **** Ok I will have to digest these new items and look in some other docs for this. Thanks.
From: Joseph M. Newcomer on 21 Jun 2010 16:30 See below... On Mon, 21 Jun 2010 15:21:51 -0400, "RB" <NoMail(a)NoSpam> wrote: > > >> You should avoid these obsolete macros. They were created before the >> C++ compiler correctly implemented (or implemented at all) the C++ >> exception syntax. >> **** >> Use try (lower case) >>> { >>> ar >> FileID; >>> if (FileID != 0x1234ABCD) >> **** > >Ok, noted, you gave me so much to work with I will have to get >back with you on most of this Actually what I am most concerned >with right now is learning all of what you just gave me. > I stepped thru my doc return code again and it appears that is not >currently doing what I had hoped. I realize if I click SAVE it is going to >save whatever new document data in currently in the members. What I >was hoping for was that the exception code would void out the current >lpszFileName doc variable so that at least when I click save it would >prompt me for a filename instead of just overwriting what is in the >lpszFileName. **** If you want your exception code to clear this variable, you will have to do that explicitly. Note that if this value is set AFTER the serialization call, because you are handling the excepiton internally and not throwing it, there is zero hope that you will be able to deal with this. Instead, you will have to add a handler for operations like File::Save, you will have to put an exception handler in that code, you will have to have your very own generic exception class (not CUserException), something like CMySerializeException, of which CWrongFilieIDException is but one of the many possible derived classes, and you will have to do a throw; instead of e->Delete() so your exception is seen outside the serialize handler. WHat I did to implement a parser was to have a CSyntaxException class and derive from it such exceptions as CUndefinedVariableException, CWrongSymbolException (e.g., finding a variable when an operator is expected), CMissingParenthesisException, CExtraParenthesisException, and things like that. CTypeMismatchException. CMissingSemicolonException (when I was pretty sure that was what was wrong). CIllegalStructMemberException. and so on and so on. Never be afraid to create your own classes of exceptions when needed. Never "hijack" some generic class. Make sure that every exception has useful information in it so you can make a report (I included file name, line #, and offset-into-line in CSyntaxException, and for things like CWrongSymbolException I had a placeholder for the entity I expected, so I would do throw new CWrongSymbolException(file, line, offset, IDS_EXPECTED_VARIABLE_NAME); class CWrongSymbolException: public CSyntaxException { public: CWrongSymbolException(const CString & file, long line, int offset, UINT Id) : CSyntaxException(file, line, offset) { symid = id; CString GetErrorString() { CString where = CSyntaxException::GetErrorString(); // filename(line) CString msg; msg.LoadString(IDS_EXPECTED); CString what; what.LoadString(symid); msg.Format(what); //filename(line): error: expected <what> return msg; } protected: UINT symid; }; Note that the explanation is a STRINGTABLE entry so I'm language-independent. Note, however, that in your code, you catch and dismiss the exception entirely within the Serialize() function, therefore, as far as anyone OUTSIDE that funtion is concerned, the exception never happened! So the assumption is that the operation completed successfully! joe ***** > >> Why is this a hardwired literal? Why did you not do >> const DWORD CURRENT_FILEID = 0x1234ABCD; >> ? >> **** > >Ugh maybe because I am a dummy and have not learned any better? > >>> { // FileID mismatch >>> AfxThrowUserException( ); >> **** >> I would have done >> throw new CWrongFileIDException(FileID) >> where I would have defined a specific exception that did exactly what >> I wanted >> class CWrongFileIDException : public CException { >> public: >> DWORD GetFileID() { return fid; } >> CWrongFileIDException(DWORD fileid) { fid = fileid; } >> protected: >> DWORD fid; >> }; >> >> You should not "hijack" some other exception to mean other than what >> precisely happened! >> **** > >Ok give me some time to work with this. > >> **** >> Query: why are you putting an English language message in SOURCE code? >> That's what STRINGTABLEs are for! > >This was only just for "me" to see if / when it was actually called. > >> Query: Why did you declare the variable E so far from where it is used? >> It should be written, if you want to keep the really bad idea of English strings, >> CString E = _T("..."); >> that is, you should never declare a variable in a scope wider than it needs to exist! > >The stuff about where to declare the vars I really just have not gotten that far yet, >but appreciate it and will try to incorporate it into my current attempt to learn >exceptions. > >> And for that matter, why did you have to declare a CString for AfxMessageBox? > >Well for the one I did not have to, but I got into the CString originally in order >to copy the strings together > > e->GetErrorMessage(szErrMsg, 255); > E = _T("RB from inside AND_CATCH\n"); > E += szErrMsg; >// Again the English is just for me to "see" when it was called. > >> ..... >> You should also do >> e->Delete(); >> to handle the case where the exception was thrown with 'new' >> **** > >Ok I will have to digest these new items and look in some other docs for this. >Thanks. 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 21 Jun 2010 16:33
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! joe On Mon, 21 Jun 2010 15:21:51 -0400, "RB" <NoMail(a)NoSpam> wrote: > > >> You should avoid these obsolete macros. They were created before the >> C++ compiler correctly implemented (or implemented at all) the C++ >> exception syntax. >> **** >> Use try (lower case) >>> { >>> ar >> FileID; >>> if (FileID != 0x1234ABCD) >> **** > >Ok, noted, you gave me so much to work with I will have to get >back with you on most of this Actually what I am most concerned >with right now is learning all of what you just gave me. > I stepped thru my doc return code again and it appears that is not >currently doing what I had hoped. I realize if I click SAVE it is going to >save whatever new document data in currently in the members. What I >was hoping for was that the exception code would void out the current >lpszFileName doc variable so that at least when I click save it would >prompt me for a filename instead of just overwriting what is in the >lpszFileName. > >> Why is this a hardwired literal? Why did you not do >> const DWORD CURRENT_FILEID = 0x1234ABCD; >> ? >> **** > >Ugh maybe because I am a dummy and have not learned any better? > >>> { // FileID mismatch >>> AfxThrowUserException( ); >> **** >> I would have done >> throw new CWrongFileIDException(FileID) >> where I would have defined a specific exception that did exactly what >> I wanted >> class CWrongFileIDException : public CException { >> public: >> DWORD GetFileID() { return fid; } >> CWrongFileIDException(DWORD fileid) { fid = fileid; } >> protected: >> DWORD fid; >> }; >> >> You should not "hijack" some other exception to mean other than what >> precisely happened! >> **** > >Ok give me some time to work with this. > >> **** >> Query: why are you putting an English language message in SOURCE code? >> That's what STRINGTABLEs are for! > >This was only just for "me" to see if / when it was actually called. > >> Query: Why did you declare the variable E so far from where it is used? >> It should be written, if you want to keep the really bad idea of English strings, >> CString E = _T("..."); >> that is, you should never declare a variable in a scope wider than it needs to exist! > >The stuff about where to declare the vars I really just have not gotten that far yet, >but appreciate it and will try to incorporate it into my current attempt to learn >exceptions. > >> And for that matter, why did you have to declare a CString for AfxMessageBox? > >Well for the one I did not have to, but I got into the CString originally in order >to copy the strings together > > e->GetErrorMessage(szErrMsg, 255); > E = _T("RB from inside AND_CATCH\n"); > E += szErrMsg; >// Again the English is just for me to "see" when it was called. > >> ..... >> You should also do >> e->Delete(); >> to handle the case where the exception was thrown with 'new' >> **** > >Ok I will have to digest these new items and look in some other docs for this. >Thanks. Joseph M. Newcomer [MVP] email: newcomer(a)flounder.com Web: http://www.flounder.com MVP Tips: http://www.flounder.com/mvp_tips.htm |