Is this class exception safe

[James Kanze]

James Kanze wrote:

[...]

I see. For the template stuff that I am dealing with, this
would be the most inconvenient convention.

This is probably the difference. The only times I use templates
are in very low level stuff. In which case, the only class
types I have to deal with are my own, or those in the standard
library; there are no other libraries below me. And of course,
the class types in my own code and in the standard library all
have a member swap. Using it (and explicitly std::swap for the
non class types) avoids the name lookup issue of calling swap
from within a function named swap.

I can see, however, that it is probably the most
convenient for you (and it makes the least assumptions).

In the concrete situations I've had to deal with to date.

 

[ExcessPhase]

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
-------------------

what is wrong with boost::any
?
See also:

http://www.boost.org/doc/html/any.html

 

[digz]

what is wrong with boost::any
?
See also:

http://www.boost.org/doc/html/any.html

I wanted something lightweight, I am always going to store ints,
doubles or strings, (if i ever need to store bigger objects i will
probably serialize them as strings and deserialize while retrieving ),
have to write lot of code around boost::any to identify types at
runtime and dispatch to the right method...

boost::variant another option( types fixed at compile time) also leads
to huge binaries, i have to write the visitation code.., compiling
requires -ftemplate-depth > 50 .something which co workers do not like
coz increased template depth .mean crazy build times..for the source
tree...
to cut a long story short keep it nice and simple.!

 

[terminator]

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;
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;
}
}

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;
}
}

I would write it this way:

struct cacheSType{
private:
union unionType{
int i_;
double d_;
char* cptr_;
};

union{//anonymous union is an object itself
unionType _cache_t;//for copying only
int i_;
double d_;
char* cptr_;
};/*all members belong to nesting block(struct cacheSType).*/

datatype _data_t;

cacheSType():_data_t(INVALID){}
cacheSType(int i): _data_t(INTEGER), _cache_t(i){}

//oops I mean:
cacheSType(int i): _data_t(INTEGER), i_(i){}

cacheSType(double d): _data_t(DOUBLE), _cache_t(d){}

//and:
cacheSType(double d): _data_t(DOUBLE), d_(d){}

cacheSType(const char* c ): _data_t(STRING){ getstr(c); };
cacheSType( const cacheSType& rhs):
_cache_t(rhs._cache_t),
_data_t(rhs._data_t){
if(_data_T==STRING)
getstr(rhs.cptr_);
};

cacheSType& operator=(const cacheSType& rhs);//defined via copy-
swap.

~cacheSType(){
if ( _data_t == STRING )
delete [] cptr_;
//just to make sure invalid data wont be used:
_data_t=INVALID;
cptr_=NULL;
};

void getstr(const char* c);//handles memory allocation

}

regards,
FM.- Hide quoted text -

- Show quoted text -

regards,
FM.

 

[terminator]

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;
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;
}
}
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;
}
}

I would write it this way:

struct cacheSType{
private:
union unionType{
int i_;
double d_;
char* cptr_;
};

union{//anonymous union is an object itself
unionType _cache_t;//for copying only
int i_;
double d_;
char* cptr_;
};/*all members belong to nesting block(struct cacheSType).*/

Dont understand why you did this ? What purpose does the anonymous
union serve ?

in order to access the union members one extra level of indirection/
encapsulation was deviced in your code and I guess this does not
increase readability nor maitenability of yor code ,So I decided to
use an anonymous union and increase readability to some extent ,but
since we were going to define a copy constructor ,I found it hard to
make an anonymous union copy constructible.That is because we can not
make sure that the alignment of all union members is the same so the
starting address of the union would be source of question ,moreover
theoretically the largest member is also not absolutely determinable.

you can see that the 'union unionType' and anonymous union have
similar structures except for the '_cache_t' member in the later. So
There is a good chance for the '_cache_t' member to have the same
size ,alignemnt and address as the containing anonymous union.this way
when defining the copy constructor you do not need to consider all the
case you just write:

you could also use accessor functions to union members instead of
using an anonymous union but I just did not like it.
In either case you did not need to define constructors for the union
in this context and that is what I wanted to avoid.

regards,
FM.

 

[terminator]

digz wrote:
[...]
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.
In this case, definitely. I might add that you can call
std::swap on his union, and the results are guaranteed to be no
throw (since a union will have a trivial copy constructor,
destructor and assignment operator, unless the user defines them
himself). In other words, he doesn't have to analyse the type
contained by the union when doing the swap.
Ok I read up the Copy and Swap Idiom , and now my assigment operator
looks like this :
Is this correct ?
cacheSType& cacheSType:perator=(const cacheSType& rhs) {
if ( &rhs != this )
{
cacheSType tmp(rhs);
std::swap(tmp._cache_t, _cache_t);

std::swap(tmp._data_t, _data_t);

}
return *this;

This looks right.

BTW: the test for self-assignment is not needed for correctness. Whether it
increases or decreases performance depends on how frequent self-assignment
is in the application.

Also: you could move the two swap lines to a swap friend function

friend
void swap ( cacheSType & rhs, cacheSType lhs ) {
std::swap( lhs._cache_t, rhs._cache_t );
std::swap( lhs._data_t, rhs._data_t );
}

making the class swapable itself is excellent ,but did you not mean to
send the 'lhs' by refrence instead of value?

void swap ( cacheSType & rhs, cacheSType& lhs )

why not to check the identity in 'swap'?
also why not a member?

void cacheSType::swap( cacheSType& lhs ){
if (this==&lhs)return;//check identity and skip
std::swap( lhs._cache_t,_cache_t );
std::swap( lhs._data_t, data_t );
};

regards,
FM.

 

[terminator]

James Kanze wrote:
[...]

The question is not how a member swap is implemented. The
question is whether a swap member function indicates that
std::swap<> is properly specialized. (Also, with regard to the
issues discusses elsethread, I would wonder how many times
std::swap<> is improperly overloaded
As I said, in my own classes, I use the member function for
class types; I don't suppose std::swap is properly specialized.

I see. For the template stuff that I am dealing with, this
would be the most inconvenient convention.

This is probably the difference. The only times I use templates
are in very low level stuff. In which case, the only class
types I have to deal with are my own, or those in the standard
library; there are no other libraries below me. And of course,
the class types in my own code and in the standard library all
have a member swap. Using it (and explicitly std::swap for the
non class types) avoids the name lookup issue of calling swap
from within a function named swap.

I can see, however, that it is probably the most
convenient for you (and it makes the least assumptions).

In the concrete situations I've had to deal with to date.

--
James Kanze (GABI Software) email:[email protected]
Conseils en informatique orientée objet/
Beratung in objektorientierter Datenverarbeitung
9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34

forgive me for not reading the whole argument about ways to define
swap,but I ask why not to force standard to be like this:

namespace std{
template <swapable T>
void swap(T& l,T&r){
if(&l!==&r)l.swap(r);
};

template<unswapable T>
void swap(T& l,T&r){
TripleAssign(l,r)
};

template<intrinsic T>
void swap(T& l,T&r);

template<assignable T>
where copyable<T>
TripleAssign(T& l,T&r){
T t(l);
l=r;
r=t;
};

template<swapable T>
swap_assign(T& dest,const T& src){
T t(src);
swap(dest,t);
};
};

in the absence of cocepts three overloads can be three distinct
templates with distinct names(just as TripleAsssign).This also lets
the programer to properly decide about the swapabitily of his class.

programmer code:

const class A& A:perator=(const A& r){
swap_assign(*this,r);
};


regards,
FM.

 

[Kai-Uwe Bux]

On Nov 11, 7:36 pm, James Kanze <[email protected]> wrote:

On Nov 11, 4:00 am, Kai-Uwe Bux <[email protected]> wrote:

digz wrote:

[...]

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.

In this case, definitely. I might add that you can call
std::swap on his union, and the results are guaranteed to be no
throw (since a union will have a trivial copy constructor,
destructor and assignment operator, unless the user defines them
himself). In other words, he doesn't have to analyse the type
contained by the union when doing the swap.

Ok I read up the Copy and Swap Idiom , and now my assigment operator
looks like this :
Is this correct ?

cacheSType& cacheSType:perator=(const cacheSType& rhs) {
if ( &rhs != this )
{
cacheSType tmp(rhs);
std::swap(tmp._cache_t, _cache_t);
std::swap(tmp._data_t, _data_t);
}
return *this;

This looks right.

BTW: the test for self-assignment is not needed for correctness. Whether
it increases or decreases performance depends on how frequent
self-assignment is in the application.

Also: you could move the two swap lines to a swap friend function

friend
void swap ( cacheSType & rhs, cacheSType lhs ) {
std::swap( lhs._cache_t, rhs._cache_t );
std::swap( lhs._data_t, rhs._data_t );
}

making the class swapable itself is excellent ,but did you not mean to
send the 'lhs' by refrence instead of value?

void swap ( cacheSType & rhs, cacheSType& lhs )

Yes, thanks.

why not to check the identity in 'swap'?

Because it's not needed for correctness. I tend to keep things simple and
only depart when profiling indicates that I should try some trickery.

also why not a member?

void cacheSType::swap( cacheSType& lhs ){
if (this==&lhs)return;//check identity and skip
std::swap( lhs._cache_t,_cache_t );
std::swap( lhs._data_t, data_t );
};

That is treated in full detail in the subthread with James Kanze.


Best

Kai-Uwe Bux

 

[Ben Rudiak-Gould]

For that matter, you can have all three: a member swap (which
seems the most natural to me), a free-standing swap in the
ambient namespace which calls it, and a specialization of
std::swap.

It's really a shame that K&R C didn't have a swap operator. It might have
had one; I think that many machines had swap opcodes, and compilers didn't
optimize very well. It would have made a lot of sense to define x <-> y to
do the same thing as the three-assignment idiom, but more efficiently. Then
this whole silly problem would never have existed. You could define
operator<-> as a member or a non-member as you liked, or omit it entirely
since the default definition would be correct for most classes (far more
than the default operator= is correct for). There would be no need for the
three-assignment std::swap, so no worries about someone calling it by
accident. It's funny how much in life hinges on syntax.

-- Ben