ma*****@yahoo.com wrote:
First, please put your responses below or inline the message you are
responding to. That's the custom here.
Here is a version of the code that works. Because i found it really
interesting (I quite a few new tricks) I thought I would post a
complete version that compiles and runs. Hopefully doing all the
changes you mentionned. I just removed const. Not sure it was really
necessary but this way i could make your first version of the code
work.
Thank you again your help -
You're welcome. More comments follow.
#include <iostream>
#include <deque>
Nit-pick: you don't use deque.
#include <map>
#include <exception>
class TException : public std::exception
{
public:
enum errorCode
{
kTest1 = 1,
kTest2 = 10,
kTest3 = 12,
kTest4 = 20
};
// error severity levels
//
// #define RIE_INFO 0
// #define RIE_WARNING 1
// #define RIE_ERROR 2
// #define RIE_SEVERE 3
enum severityLevel
{
kInfo = 0, // Rendering stats and other info
kWarning, // Something seems wrong, maybe okay
kError, // Problem. Results may be wrong
kSevere // So bad you should probably abort
};
// error handlers codes
//
// #define RIE_ERRORIGNORE 0
// #define RIE_ERRORPRINT 1
// #define RIE_ERRORABORT 2
enum handlerCode
{
kErrorIgnore = 0,
kErrorPrint,
kErrorAbort,
};
typedef std::map<TException::errorCode, const char*MsgMap;
You don't need to qualify errorCode or other member types with the
current class name. This could more simply be:
typedef std::map<errorCode, const char*MsgMap;
>
public:
This "public" is redundant, though you might prefer it for aesthetic
reasons.
TException( TException::errorCode code, TException::handlerCode
handler =
kErrorIgnore, TException::severityLevel severity = kInfo ) :
Again, no need to qualify member types in this context.
m_error( code ),
m_severity( severity ),
m_handler( handler )
{}
~TException() throw() {}
This is unnecessary. The compiler will automatically generate the same
code for you, so you may as well delete it.
>
const char* what() const throw()
{
return myMsgMap[ m_error ];
}
This function is much inferior to the one I presented previously. Here
is mine again for reference:
virtual const char* what() const throw()
{
const MsgMap::const_iterator iter = myMsgMap.find( m_code );
if( iter != myMsgMap.end() )
{
return iter->second;
}
return "No error message found!";
}
The chief difference is this: yours could return a null pointer, while
mine cannot. Try this evil but perfectly legal code with your existing
class and see what happens:
throw TException( TException::errorCode( 17 ) );
When mine is called with an invalid m_code (enums aren't guaranteed to
prevent every error, after all), it returns "No error message found!".
When yours is called with the same parameter, it returns the result of
this expression (cf.
http://www.sgi.com/tech/stl/Map.html#3):
(*((myMsgMap.insert(
MsgMap::value_type(k, MsgMap::data_type()))).first)).second
You'll note that it calls std::map's insert() member function, which is
non-const. This version of the function returns an iterator pointing to
the current element with the specified key if it already exists *but*
inserts a new key/data pair with the default-constructed data if it
doesn't already exist. (It also returns a bool indicating if the key
was found or not, but that is ignored here.) The default construction
of a pointer is a null pointer, and chances are your client assumes
std::exception::what() returns a non-null pointer, which is probably
immediately dereferenced, and bang! you're dead.
The proper way to prevent this is to declare myMsgMap as a const static
member and use the map<>::find() member function, as I did in my
previous installment.
>
private:
TException::errorCode m_error;
TException::handlerCode m_handler;
TException::severityLevel m_severity;
static MsgMap myMsgMap;
These should all be const since you don't want them getting mutated by
any member functions (accidentally or on purpose!). See
<http://www.parashift.com/c++-faq-lite/const-correctness.html>. Note
that the *non-static* members are all implicitly made const within the
context of TException::what() because it is a const member function
(see FAQ 18.10), but you should still declare them const here since it
makes the code "easier to understand, track, and reason about" (Sutter
and Alexandrescu, _C++ Coding Standards_, Item 15) because the
programmer's intention is clear.
Moreover, a const member function does not affect the constness of
static data members such as myMsgMap. This means, obviously, that your
map is modifiable even in const functions. The proper solution that
avoids altering the map, which shouldn't change after start up, is to
keep myMsgMap const (as I had in my original code) but use the what()
function body given above since it only utilizes const member functions
of std::map.
};
class MsgMapInitializer
{
TException::MsgMap m_;
public:
operator TException::MsgMap() const { return m_; }
MsgMapInitializer& Add( TException::errorCode error, const char * msg
)
{
m_[ error ] = msg;
return *this;
}
};
TException::MsgMap TException::myMsgMap = MsgMapInitializer()
.Add( TException::kTest1, "test1" )
.Add( TException::kTest2, "test2" )
.Add( TException::kTest3, "test3" );
int main()
{
try {
// throw something
throw( TException( TException::kTest1, TException::kErrorPrint,
TException::kWarning ) );
} catch( TException &e ) {
Catch a const exception when possible (const-correctness again):
catch( const TException &e )
You could also catch a "const std::exception&" here instead since you
inherit from it and don't use any extra members from TException in the
exception handler.
std::cout << e.what() << std::endl;
};
return 0;
}
Cheers! --M
