Re: Is this class exception safe

From:

Kai-Uwe Bux <jkherciueh@gmx.net>

Newsgroups:

comp.lang.c++

Date:

Sat, 10 Nov 2007 19:00:20 -0800

Message-ID:

<fh5r8s$rd1$1@murdoch.acc.Virginia.EDU>

digz wrote:

Hi ,
I am trying to write a class that encapsulates a Union within it , I
intend the code to be exception safe.
can someone please review it and tell me if I have done the right
thing , are there any obvious memory leaks or bad design , inability
to cover most likely failure paths etc..?.

Thanks in advance
Digz
-------------------

#include<exception>
enum datatype { INVALID=-1, INTEGER=1, STRING, DOUBLE };

struct cacheSType
{
    private:
        union cacheUType
        {
            int i_;
            double d_;
            char* cptr_;
            cacheUType():d_(-999){}
            cacheUType(int i):i_(i){}
            cacheUType(double d):d_(d){}
            cacheUType(char* c):cptr_(c){}
        };
        typedef cacheUType unionType;

Is there a reason to use a typedef instead of just saying

union unionType {...

        datatype _data_t;
        unionType _cache_t;

    public:
        cacheSType():_data_t(INVALID){}
        cacheSType(int i): _data_t(INTEGER), _cache_t(i){}
        cacheSType(double d): _data_t(DOUBLE), _cache_t(d){}
        cacheSType(const char* c ): _data_t(STRING){
            try {
            _cache_t.cptr_ = new char[strlen(c) + 1 ];
            strcpy(_cache_t.cptr_, c);
            }
            catch( const std::bad_alloc &oom ){
                cerr << oom.what() << endl;
                throw;
            }
            catch(...) {
                throw;
            }

I am not sure whether catch(...){throw;} does anything. It seems to just
rethrow an exception that otherwise would have propagates up the stack
anyway.

        }

        cacheSType( const cacheSType& rhs) {
            try {
                if ( rhs._data_t == STRING ) {
                    _cache_t.cptr_ = new
char[strlen(rhs._cache_t.cptr_) + 1];
                    strcpy(_cache_t.cptr_, rhs._cache_t.cptr_);
                }
                else {
                  _cache_t = rhs._cache_t;
                }
                _data_t = rhs._data_t;
            }
            catch( const std::bad_alloc &oom) {
                cerr << oom.what() << endl;
                throw;
            }
            catch(...) {
                throw;
            }
        }

        cacheSType& operator=(const cacheSType& rhs) {
            if ( &rhs != this ) {
                if ( rhs._data_t != STRING ) {
                    if ( _data_t == STRING)
                        delete [] _cache_t.cptr_;
                   _cache_t = rhs._cache_t;
                }
                else{
                    if ( _data_t == STRING )
                        delete [] _cache_t.cptr_;
                    try {
                    _cache_t.cptr_= new
char[strlen(rhs._cache_t.cptr_) + 1];
                    }
                    catch( const std::bad_alloc &oom) {
                        cout << oom.what() << endl;
                        throw;
                    }
                    catch(...) {
                        throw;
                    }

If the above throws, you have already deleted the current string. The object
is then in an inconsistent state since the _cache_t says it's a string, but
the string-field points to deallocated memory. In that sense, the
assignment operator is not exception safe.

Also, the logic of the assignment operator is hard to follow (after all,
there are 9 possible combinations of _data_t and rhs._data_t to consider).
For clarity and exception safety, you should consider using the copy-swap
idiom.

                    strcpy( _cache_t.cptr_, rhs._cache_t.cptr_);
                }
                   _data_t = rhs._data_t;
            }
            return *this;
        }

        operator int () {
            if ( _data_t == INTEGER )
                return _cache_t.i_;
            throw std::bad_cast(); //cannot return anything sensible
if not int
        }

        operator double () {
            if ( _data_t == DOUBLE )
                return _cache_t.d_;
            throw std::bad_cast();
        }

        operator const char* () {
            if ( _data_t == STRING )
                return _cache_t.cptr_;
            throw std::bad_cast();
        }

        ~cacheSType(){
            if ( _data_t == STRING )
                delete [] _cache_t.cptr_;
         }

}

Best

Kai-Uwe Bux