Is this class exception safe

[digz]

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

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

}

 

[Kai-Uwe Bux]

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

 

[James Kanze]

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.

 

[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){}
cacheSType(double d): _data_t(DOUBLE), _cache_t(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.

 

[digz]

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);
data_t = rhs.data_t;
}
return *this;
}

 

[digz]

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 ?

datatype _data_t;

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

}

Thx
Digz

 

[digz]

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;



}- Hide quoted text -

Added the extra line std::swap(tmp._data_t, _data_t ) in the
assignment operator
Had to swap the _data_t otherwise tmp was in an inconsistent state ,
and the tmp destructor call was segfaulting
If there is still something wrong please let me know
Thanks very much for all your comments.
--Digz

 

[Kai-Uwe Bux]

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

and then call that in the assignment operator. This swap() could then be
used in other places instead of std::swap<cacheSType>, which would call
your assignment-operator three times resulting in 6 member-swaps as opposed
to only two.

Added the extra line std::swap(tmp._data_t, _data_t ) in the
assignment operator
Had to swap the _data_t otherwise tmp was in an inconsistent state ,
and the tmp destructor call was segfaulting
If there is still something wrong please let me know

Best

Kai-Uwe Bux

 

[James Kanze]

Kai-Uwe Bux wrote:

[...]

BTW: the test for self-assignment is not needed for correctness.

In general (there are probably exceptions), the need for a test
for self-assignment is often a sign that the assignment operator
is not excetpion safe (but as you point out, it's not necessary
here).

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

and then call that in the assignment operator. This swap()
could then be used in other places instead of
std::swap<cacheSType>, which would call your
assignment-operator three times resulting in 6 member-swaps as
opposed to only two.

I think it would be more idiomatic to have a swap member
fucntion, and specialize std::swap to use it. I usually assume
that if a class does not have a swap member function, std::swap
will be done using the "standard" algorithm, with copy
construction and assignment, and don't use the swap idiom if my
class contains members of class type which don't have a swap
member function.

 

[Kai-Uwe Bux]

Kai-Uwe Bux wrote:

[...]

BTW: the test for self-assignment is not needed for correctness.

In general (there are probably exceptions), the need for a test
for self-assignment is often a sign that the assignment operator
is not excetpion safe (but as you point out, it's not necessary
here).

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

and then call that in the assignment operator. This swap()
could then be used in other places instead of
std::swap<cacheSType>, which would call your
assignment-operator three times resulting in 6 member-swaps as
opposed to only two.

I think it would be more idiomatic to have a swap member
fucntion, and specialize std::swap to use it.

I disagree (or at least I would contend that what is idiomatic in this area
is shifting). I just put a swap() function into the same namespace as the
class. Then name-lookup will find it. Note that the next version of the
standard makes this (and not a swap member function) one of the two ways to
satisfy the swappable requirement.

I usually assume
that if a class does not have a swap member function, std::swap
will be done using the "standard" algorithm, with copy
construction and assignment,

That assumption could also be correct for classes that have a swap member.
You are heavily relying on (local) coding conventions should you assume
that std::swap is specialized for classes that have a swap member.

I follow the convention not to use std::swap, but swap (usually after making
std::swap visible via "using"). This way, I can swap everything swappable.
I think next version of the standard will ensure that standard algorithm
will use swap and not std::swap so that I specializing std::swap is
superfluous.

and don't use the swap idiom if my
class contains members of class type which don't have a swap
member function.

I just assume that members are swappable. _If_ the swap is no-throw for
_all_ members, then so is mine (and copy-swap-assignment will be exception
safe).


Best

Kai-Uwe Bux

 

[Alf P. Steinbach]

* Kai-Uwe Bux:

I follow the convention not to use std::swap, but swap (usually after making
std::swap visible via "using"). This way, I can swap everything swappable.
I think next version of the standard will ensure that standard algorithm
will use swap and not std::swap so that I specializing std::swap is
superfluous.

Well, currently it's formally invalid to specialize any function in
namespace std: classes yes, functions no, njet, nein, nei, nope. Very
stupid rule. I just ignore it for my own private small programs. ;-)

Cheers,

- Alf

 

[Kai-Uwe Bux]

* Kai-Uwe Bux:

Well, currently it's formally invalid to specialize any function in
namespace std: classes yes, functions no, njet, nein, nei, nope. Very
stupid rule. I just ignore it for my own private small programs. ;-)

I thought so, too; and I was about to mention it. However, then I looked up
the wording:

A program may add template specializations for any standard library
template to namespace std.

Now, I used to think that one cannot provide alternative specializations for
function templates (all one can do is to add overloads). Then, I used the
pdf-reader to search for specializations and found that the standard
acknowledges the existence of function template specializations (oops). At
that point, I was not sure anymore whether my previous thinking was correct
(it's kind of late). That's why I dropped the issue.


Best

Kai-Uwe Bux

 

[Andre Kostur]

* Kai-Uwe Bux:

Well, currently it's formally invalid to specialize any function in
namespace std: classes yes, functions no, njet, nein, nei, nope. Very
stupid rule. I just ignore it for my own private small programs. ;-)

Chapter and verse?

I found 17.4.3.1.1 which specifically mentions: "A program may add template
specializations for any standard library template to namespace std." This
seems to specifically allow one to specialize std::swap for one's own
classes.

 

[Alf P. Steinbach]

* Kai-Uwe Bux:

I thought so, too; and I was about to mention it. However, then I looked up
the wording:

A program may add template specializations for any standard library
template to namespace std.

Now, I used to think that one cannot provide alternative specializations for
function templates (all one can do is to add overloads). Then, I used the
pdf-reader to search for specializations and found that the standard
acknowledges the existence of function template specializations (oops)

Complete specialization, yes, partial no -- what looks like a partial
specialization (and what I meant by "specialization" above, using the
terminology of the article I responded to) is just an overload.

Andrei Alexandrescu and I think it was Herb Sutter or perhaps it was
Petru Marginean wrote an article discussing this at length, including
examples of counter-intuitive effects, available on the net.

Nothwitstanding that, Pete Becker wrote an article in DDJ about the TR1
where he used the term "specialization" in the way I/we did above, and
as I recall also the standard uses this wording in at least one place.

. At
that point, I was not sure anymore whether my previous thinking was correct
(it's kind of late). That's why I dropped the issue.

Cheers, -- did you mean "late", or did you mean "early"?

- Alf

 

[Alf P. Steinbach]

* Andre Kostur:

Chapter and verse?

I found 17.4.3.1.1 which specifically mentions: "A program may add template
specializations for any standard library template to namespace std." This
seems to specifically allow one to specialize std::swap for one's own
classes.

That only covers template specializations.

Here's a template specialization of std::swap:

namespace std
{
template<> void swap( MyClass& a, MyClass& b ) { a.swap( b ) }
}

and that is OK, but here's an overload of std::swap:

namespace std
{
template< typename T >
void swap( MyClass<T>& a, MyClass<T> b ) { a.swap( b ); }
}

and that's formally ungood.

Because 17.4.3.1.1 is not much use for function overloads.

Re the article you responded to, I just wrote with Kai-Uwe in mind,
forgot this is a public newsgroup. Change "specialization" to
"overload". Also look up Andrei's article about this if the difference
isn't clear (why second example above is not a specialization).

Cheers, & hth.,

- Alf

 

[James Kanze]

Kai-Uwe Bux wrote:
[...]

BTW: the test for self-assignment is not needed for correctness.

In general (there are probably exceptions), the need for a test
for self-assignment is often a sign that the assignment operator
is not excetpion safe (but as you point out, it's not necessary
here).

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 );
}
and then call that in the assignment operator. This swap()
could then be used in other places instead of
std::swap<cacheSType>, which would call your
assignment-operator three times resulting in 6 member-swaps as
opposed to only two.

I think it would be more idiomatic to have a swap member
fucntion, and specialize std::swap to use it.

I disagree (or at least I would contend that what is idiomatic
in this area is shifting). I just put a swap() function into
the same namespace as the class. Then name-lookup will find
it.

Not if the code in question wrote std::swap.

FWIW: my argument that it is "idiomatic" is based on the fact
that this is what the standard containers are required to do.
And as far as I know, that isn't changing.

Note that the next version of the standard makes this (and not
a swap member function) one of the two ways to satisfy the
swappable requirement.

And the current version allows std::sort to use std::swap.

Obviously, a swap member function alone doesn't suffice---a
template designed to work with non-class types as well as class
types can't use it. A swap member function called by a
specialization of std::swap will work, and seems the safest
solution at present. The presence of a swap member function
makes it quite clear to the reader that your class does support
a specialized swap. (Of course, if the class is correctly
documented, that's how the user knows what's what.)

That assumption could also be correct for classes that have a
swap member.

It could be.

You are heavily relying on (local) coding conventions should
you assume that std::swap is specialized for classes that have
a swap member.

I'm relying mainly on experience with a lot of code from a lot
of places. I've yet to see a swap member function that invoked
std::swap( *this, other ).

I follow the convention not to use std::swap,

But others might not. You don't control the code in std::sort.

but swap (usually after making
std::swap visible via "using"). This way, I can swap everything swappable.
I think next version of the standard will ensure that standard algorithm
will use swap and not std::swap so that I specializing std::swap is
superfluous.

I know that the issue is being discussed; I don't know what the
final decision is. Not that it matters; it will be some years
before I get to use a compiler that is compatible with the next
version of the standard. (Of the compilers/libraries I
currently use, two use unqualified swap, one uses std::swap, and
one does the swap manually (!), so you loose either way you do
it.)

 

[James Kanze]

* Andre Kostur:

That only covers template specializations.

Here's a template specialization of std::swap:

namespace std
{
template<> void swap( MyClass& a, MyClass& b ) { a.swap( b ) }
}

and that is OK,

And that's what we're talking about.

 

[Kai-Uwe Bux]

Kai-Uwe Bux wrote:
[...]
BTW: the test for self-assignment is not needed for correctness.
In general (there are probably exceptions), the need for a test
for self-assignment is often a sign that the assignment operator
is not excetpion safe (but as you point out, it's not necessary
here).
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 );
}
and then call that in the assignment operator. This swap()
could then be used in other places instead of
std::swap<cacheSType>, which would call your
assignment-operator three times resulting in 6 member-swaps as
opposed to only two.
I think it would be more idiomatic to have a swap member
fucntion, and specialize std::swap to use it.

I disagree (or at least I would contend that what is idiomatic
in this area is shifting). I just put a swap() function into
the same namespace as the class. Then name-lookup will find
it.

Not if the code in question wrote std::swap.

FWIW: my argument that it is "idiomatic" is based on the fact
that this is what the standard containers are required to do.
And as far as I know, that isn't changing.

On the other hand, the standard has no place where it makes the existence of
a swap member function a conceptual requirement. We have to distinguish two
questions:

a) Is it advisable to specialize std::swap<> ?
b) Should the local swap be a member function or a free-standing function?

I tend to agree with (a) but not with (b).

And the current version allows std::sort to use std::swap.

Obviously, a swap member function alone doesn't suffice---a
template designed to work with non-class types as well as class
types can't use it. A swap member function called by a
specialization of std::swap will work, and seems the safest
solution at present. The presence of a swap member function
makes it quite clear to the reader that your class does support
a specialized swap. (Of course, if the class is correctly
documented, that's how the user knows what's what.)

Note: specializing std::swap<> and having a swap member function (as opposed
to a free-standing swap in the ambient namespace) are separate issues. If
needed, I can still specialize std::swap< my_namespace::my_class > to use
my_namespace::swap.

It could be.


I'm relying mainly on experience with a lot of code from a lot
of places. I've yet to see a swap member function that invoked
std::swap( *this, other ).

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

Note: by saying

using std::swap;

and then consistently using unqualified swap(), the code takes advantage of
any class that is swappable and of any class that specialized std::swap.

But others might not. You don't control the code in std::sort.

See above: specializing std::swap<> is still an option; and you might be
right to recommend it.

I know that the issue is being discussed; I don't know what the
final decision is. Not that it matters; it will be some years
before I get to use a compiler that is compatible with the next
version of the standard. (Of the compilers/libraries I
currently use, two use unqualified swap, one uses std::swap, and
one does the swap manually (!), so you loose either way you do
it.)

Nice data points.


Best

Kai-Uwe Bux

 

[James Kanze]

James Kanze wrote:
Kai-Uwe Bux wrote:
[...]

FWIW: my argument that it is "idiomatic" is based on the fact
that this is what the standard containers are required to do.
And as far as I know, that isn't changing.

On the other hand, the standard has no place where it makes
the existence of a swap member function a conceptual
requirement.

We have to distinguish two questions:

a) Is it advisable to specialize std::swap<> ?
b) Should the local swap be a member function or a free-standing function?

I tend to agree with (a) but not with (b).

Just curious, but why not? (Although it's an interesting
question. As a general rule, std::swap is defined to have the
semantics I want, an otherwise visible swap theoretically might
not (although I can't imagine anything else swap could mean off
hand). So I would normally have automatically used std::swap
everywhere. The standard seems to be going in the other
direction, however: requiring that the call to swap in the
library not be qualified (and thus that it might pick up a swap
with other semantics---or rather, indirectly, that any function
named swap must have the semantics specified by the standard,
regardless of what namespace it is in).

Note: specializing std::swap<> and having a swap member
function (as opposed to a free-standing swap in the ambient
namespace) are separate issues.

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.

Given the variation in practice here, that seems to be the
safest.

Note that my convention is more or less an adaptation to the all
to frequent case of poor documentation. My experience has been
that programmers don't provide a member function swap unless
they are aware of the swap idiom, and providing for it. So it's
presence is a strong indication of something that really should
be documented. If your class documentation otherwise specifies
that the class supports the swap idiom by means of a global
function, that's fine too. (If you're using something like
Doxygen, it's probably easier to document the member function,
and have the documentation show up in the right place. This is
probably best considered a weakness of Doxygen---or of my
knowledge of Doxygen---but it doesn't associate free functions
with the relevant class.)

[...]

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.
It's nothing cast in stone, but it happens to be the strategy
which seems to work best with the code I've actually seen, and
had to deal with. (In the meantime, I think I will take to
adding a local overload of swap as well. No point in not
covering all bases.)

 

[Kai-Uwe Bux]

James Kanze wrote:
Kai-Uwe Bux wrote:
[...]
FWIW: my argument that it is "idiomatic" is based on the fact
that this is what the standard containers are required to do.
And as far as I know, that isn't changing.

On the other hand, the standard has no place where it makes
the existence of a swap member function a conceptual
requirement.

We have to distinguish two questions:

a) Is it advisable to specialize std::swap<> ?
b) Should the local swap be a member function or a free-standing
function?

I tend to agree with (a) but not with (b).

Just curious, but why not?

Ah, that goes back to the hassles that I called upon me when I tried to
write generic code that would _call_ the right swap for each type. To see
the problem, recall that there are (at least) three ways for a type T to
provide a specialized swap:

a) specialize std::swap<>
b) define a swap member function
c) define a free-standing swap (presumably a friend)

Now, when I write my template code based on T, how should I do a swap? I can
take care of (a) and (c) in one swoop by making std::swap visible in my
namespace. The swap from the namespace of T will be visible through ADL.
But, how should I take care of the member functions? I tried this: use
template magic to detect a swap member. However, it turned out to be a
royal pain because I it turned swap into something ambiguous. In the end, I
had to write

call_swap( lhs, rhs );

in my own code instead of swap just so that call_swap() would magically
resolve to the right thing. At that point, I decided that it isn't worth
the effort to support all three conventions. Since a) and c) are the
easiest to support simultaneously, I went with that.

Another reason is my own lazyness. If I do:

template < typename T >
class X {

T member;

void swap ( X & other ) {
swap( this->member, other.member );
...
}

};

I run into the issue that the local swap member hides the identifier for the
swap function that is being called. Since I want the swap call to resolve
correctly, and since I am dealing with a templated type, I cannot just go
on an put a namespace qualification.

If I do:

template < typename T >
class X {

T member;

friend
void swap ( X & lhs, X & rhs ) {
swap ( lhs.member, rhs.member );
}

};

the issue does not come up.


In short, I found that in template code (which is most of what I do), the
convention (c) is the easiest to deal with.

(Although it's an interesting
question. As a general rule, std::swap is defined to have the
semantics I want, an otherwise visible swap theoretically might
not (although I can't imagine anything else swap could mean off
hand). So I would normally have automatically used std::swap
everywhere. The standard seems to be going in the other
direction, however: requiring that the call to swap in the
library not be qualified (and thus that it might pick up a swap
with other semantics---or rather, indirectly, that any function
named swap must have the semantics specified by the standard,
regardless of what namespace it is in).

Yup that seems to be the direction of the standard. I wholeheartly agree
with that

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.

Given the variation in practice here, that seems to be the
safest.
Probably.


Note that my convention is more or less an adaptation to the all
to frequent case of poor documentation. My experience has been
that programmers don't provide a member function swap unless
they are aware of the swap idiom, and providing for it. So it's
presence is a strong indication of something that really should
be documented. If your class documentation otherwise specifies
that the class supports the swap idiom by means of a global
function, that's fine too. (If you're using something like
Doxygen, it's probably easier to document the member function,
and have the documentation show up in the right place. This is
probably best considered a weakness of Doxygen---or of my
knowledge of Doxygen---but it doesn't associate free functions
with the relevant class.)

Right: freestanding swap functions are clearly part of the class interface
and need to be documented as such.

On a related note: generic libraries should clearly document whether they
require a type to have a swap member function, be swappable, or use a
specialized std::swap.

[...]

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. I can see, however, that it is probably the most
convenient for you (and it makes the least assumptions).

It's nothing cast in stone, but it happens to be the strategy
which seems to work best with the code I've actually seen, and
had to deal with. (In the meantime, I think I will take to
adding a local overload of swap as well. No point in not
covering all bases.)

Yup.


Best

Kai-Uwe Bux