Printable View
I've made the copy constructor private in one of my classes and i'm getting a compiler error informing me that std::swap is trying to call the copy constructor.
is there a way to make the std::swap a friend of the class? I couldn't get it to do it.
I tried:
but gcc tells me swap should've been declared in std.Code:friend void std::swap( MyClass&, MyClass& )
Any ideas?
BTW: this is the swap in <algorithm> if it makes any difference
maybe something like:
Code:class Myclass {
Myclass(const Myclass&);
public:
void swap(Myclass& other) {
// Your implementation of swap here
}
};
namespace std {
void swap(Myclass& left, Myclass &right) {
left.swap(right);
}
}
Well I'd rather not re-implement "swap" since the one in <algorithm> does exactly what I want it to.
and why would i want to declare another namespace called std?
Why not implement a MyClass::swap()? That's how it's typically handled.
Because it's being called by another class which has a container of the class in question. Basically like this:Quote:
Originally Posted by brewbuck
a shuffling method if you willCode:class Foo{
private:
Foo( const Foo& );
};
class Bar{
std::vector<Foo> foo_vec;
public:
void fxn(){
for( int i = 0; i < foo_vec.size(); i++ )
std::swap( foo_vec[i], foo_vec[get_rand_int(0,foo_vec.size())]);
}
Why is the copy constructor private?
If you use brewbuck's idea then you just change it to:
but it sounds fishy that you can swap but not copy.Code:foo_vec[i].swap(foo_vec[get_rand_int(0,foo_vec.size())]);
Then you should provide a specialization of std::swap() which will just invoke your MyClass::swap().Quote:
Originally Posted by dudeomanodude
No it doesn't. This is what std::swap does:Quote:
Originally Posted by dudeomanodude
Which calls the copy constructor (And fails if it's private, as in your example)Code:template<class T>
void swap(T& left, T& right) {
T tmp = left;
left = right, right = tmp;
}
If you implement a swap method, you don't suffer from this problem (And you can also avoid extra copies)
I'm not declaring another namespace std. I'm extending the already existing namespace with the specialized swap<Myclass>Quote:
and why would i want to declare another namespace called std?
I think I'll just reimplement the copy ctor to avoid what it is i'm trying to avoid...Quote:
Why is the copy constructor private?
If you use brewbuck's idea then you just change it to:
Code:
foo_vec[i].swap(foo_vec[get_rand_int(0,foo_vec.size())]);
but it sounds fishy that you can swap but not copy.
Remember to implement the copy assignment operator as well. Actually, you might as well go ahead with the advice to implement a member swap and then implement the copy assignment operator in terms of the copy constructor, member swap and destructor... and also specialise std::swap using your member swap.Quote:
Originally Posted by dudeomanodude
Two things:
1. I implemented a member swap, and it still fails with the private copy ctor, even though the class calling it is a friend. Example:
I really do need a private copy ctor/assignment optor because (in my actual program)Foo holds a pointer to Bar, which identifies the Foo's origin. If a Bar makes a Foo, the Foo get's the Bar's this-> pointer, if a Foo is created on its own, it's Bar pointer stays null. Now the problem I'm having is that if I overload Foo's copy ctor to make the Bar pointer null, then the implementation of shuffle is completely hosed. Any ideas for this situation are most appreciated.Code:class Foo{
Foo( const Foo& );
friend class Bar;
public:
void swap( Foo& right ){
Foo temp = *this;
*this = right, right = temp;
}
};
class Bar{
public:
void shuffle( Foo& left, Foo& right ){
left.swap( right );
}
};
2. Where do I extend std::swap? I get a multiple definition error if I try to in Foo's header. I've never done that before, so is there any guidelines with extending namespaces in this fashion?
Friendship no longer matters since the member swap is a public member function. Instead of implementing the member swap in terms of the copy constructor and copy assignment operator, implement it to actually swap the members. You are probably getting linker errors at the moment because the copy constructor is declared but not implemented (and you forgot to declare the copy assignment operator as private).Quote:
Originally Posted by dudeomanodude
I do not get your reasoning. The copy constructor would not make the Bar pointer null, but would make the bar pointer a copy of the bar pointer of the source Foo object.Quote:
Originally Posted by dudeomanodude
Do something like this:Quote:
Originally Posted by dudeomanodude
Code:namespace std
{
template<>
void swap<Foo>(Foo& a, Foo& b)
{
a.swap(b);
}
}
No. I've declared both, and implemented both.Quote:
You are probably getting linker errors at the moment because the copy constructor is declared but not implemented (and you forgot to declare the copy assignment operator as private).
And this is exactly what I'm trying to avoid. If copying goes public, than yes, the copy semantics need to be re-written to make the Bar* null. I don't want to be able to copy a Foo from an existing Foo, and take it's Bar*( its originator ) along with it. Bar has a container of Foos. Bar can give out these Foos, it can also take them back. But it can only take them back if it came from it. That's the situation I'm trying to program here. So copying becomes a problem, especially now that I want to call swap. Before implementing shuffle, private copying was fine. Now, I've programmed myself into a corner.Quote:
I do not get your reasoning. The copy constructor would not make the Bar pointer null, but would make the bar pointer a copy of the bar pointer of the source Foo object.
Don't implement the swap method in terms of copy constructor (which std::swap already does). Instead, swap around the members contained within Foo. For example, a string would swap the pointers to the internal buffer (as well as the other data members) without constructing a third string.
--------------
As a side note, the following compiles with VC++ 2005, but not with MinGW 3.4.5. The latter uses std::iter_swap (which performs a swap using copy constructor) in std::random_shuffle and not std::swap. Is there anything that can be done about it and why on earth did they even need the iter_swap algorithm in the standard.
Code:#include <algorithm>
class X
{
X(const X&);
public:
X() {}
void swap(X&) {}
};
namespace std
{
template <>
void swap(X& a, X& b)
{
a.swap(b);
}
}
int main()
{
X x[10];
std::random_shuffle(x, x + 10);
}
Okay, this makes things a little clearer. What do you mean by "give out these Foos" and "take them back"? Are you talking about copying semantics similiar to std::auto_ptr? What does it mean to swap two Foo objects? Why does Bar::shuffle merely swap two Foo objects apparently unrelated to the Bar object?Quote:
Originally Posted by dudeomanodude
Okay, I'm with you on that one.Quote:
Don't implement the swap method in terms of copy constructor (which std::swap already does). Instead, swap around the members contained within Foo. For example, a string would swap the pointers to the internal buffer (as well as the other data members) without constructing a third string.
So, basically I can't extend std::swap if I plan on keeping copying private?Quote:
As a side note, the following compiles with VC++ 2005, but not with MinGW 3.4.5.
And to laserlight,
Imagine a deck of cards if you will. You can hand those cards out, and you can put them back in the deck. However, i don't want to put cards back that came from some other deck or elsewhere.
I'm still getting the privacy issues even after implementing the member swap to swap around members:
What am I doing wrong that I still get the "blah is private within this context" error?Code:void Foo::m_swap( Foo& f ){
int temp_value = this->m_value;
this->m_value = f.m_value;
f.m_value = temp_value;
}
EDIT: the error is still citing "Foo::Foo( const Foo& )" as the issue? I thought I was avoiding copying now? I'm lost.
You can, but in my opinion you should not. As I mentioned, I find it fishy that one can swap and yet not copy.Quote:
Originally Posted by dudeomanodude
So, copying a card does not make sense since only one such card may exist in a deck. One solution is to simply disable copying entirely and work with card pointers instead. You would not have a swap function, but instead just swap pointers. The deck of cards would be represented by a vector of card pointers; this vector can be shuffled as needed. My worry would be ownership: who would be responsible for destroying the card objects that the pointers point to? You need to decide on that if you take this path. (Maybe work with std::tr1::shared_ptrs, for example.)Quote:
Originally Posted by dudeomanodude
That said, consider if you really do need to enforce the association of cards with the deck: if only one deck is ever in use at any point, there would be no need to check for such an association.
Post the smallest and simplest code that demonstrates the error.Quote:
Originally Posted by dudeomanodude
You may need to cite the entire error message.
Actually, are you saying that you have a vector of non-copyable items, since I thought that a requirement of containers is that the contents need to be copyable (or pehaps you are getting around that by using a public assignment operator - in which case hiding copy constructor seems a bit meaningless)?
When I made a black-jack game, my Deck didn't physically remove cards from it. Instead it kept an index to the first unused card (and dealt copies of cards). Resetting the deck simply meant setting this index back to 0 and reshuffling.Quote:
Imagine a deck of cards if you will. You can hand those cards out, and you can put them back in the deck. However, i don't want to put cards back that came from some other deck or elsewhere.
Either shuffle pointers, or do it the way anon says, by allowing copies. Making non-copyable object but then allowing them to be swapped is a design error.
iter_swap is a mere convenience function. It used to be specified as "Exchanges the values pointed to by the two iterators a and b." (C++03 25.2.2p7), but this was found to be underspecified and was replaced with "swap(*a, *b)" (C++0x Draft N2798 25.2.3p8).
Still underspecified in my opinion. It should be made clear that it finds swap overloads by ADL.
Well I have a lot to consider here. My original idea (whether anyone finds it useful is beside the point), was to create a generic representation of cards, and a deck of cards that would allow a user to create/recreate any card game. A card game API if you will (I'm only doing this for my own amusement/learning).
Container of card pointers:
yea, ownership was always what I was worried about, which is why i didn't take that path. However, as anon has suggested, simply keeping an index to represent the next available card would eliminate the "ownership" and handing out cards all together. That's probably what I'll do.
There's some gotchas though if you consider cards and decks of cards as being generic. Once cards are shuffled, I only need one method to draw a card(for every card game I can think of). But putting cards back becomes a problem. What if one game wants you to put cards on the top of the deck, another to the bottom, and yet another places cards back on a random heap. So putting cards back isn't exactly trivial. Those problems seemed to be easily solved by physically removing cards from the deck and keeping track of which deck they came from. That is until the whole swap issue came about.
I haven't got there yet, but next will be card values. Card values aren't trivial either since different games will need to value cards differently. I'm thinking of mapping the cards out and allowing easy ways for someone to assign values by referring to all cards of a particular face value, suit, or individually.
There's a lot to consider, and it's not that I expect anyone to use it, except maybe me, it's just for fun.
Well, MinGW implements it like this:
Whereas MSVC uses std::swap:Code:template<typename _ForwardIterator1, typename _ForwardIterator2>
inline void
iter_swap(_ForwardIterator1 __a, _ForwardIterator2 __b)
{
typedef typename iterator_traits<_ForwardIterator1>::value_type
_ValueType1;
typedef typename iterator_traits<_ForwardIterator2>::value_type
_ValueType2;
// concept requirements
//...
const _ValueType1 __tmp = *__a;
*__a = *__b;
*__b = __tmp;
}
I'm really tempted to modify MinGW's stl_algobase.h since it doesn't even attempt to use swap...Code:template<class _FwdIt1,
class _FwdIt2> inline
void iter_swap(_FwdIt1 _Left, _FwdIt2 _Right)
{ // swap *_Left and *_Right
std::swap(*_Left, *_Right);
}
Oh god, they're both wrong. The only correct implementation, that won't break code, is
GCC 4.4 implements this correctly.Code:template <typename FwdIt1, typename FwdIt2> inline
void iter_swap(FwdIt1 it1, FwdIt2 it2)
{
swap(*it1, *it2); // iter_swap is in namespace std, so it can find std::swap,
// and ADL overloads will be found too.
}
That does not make sense: std::swap requires that its arguments be of the same type, but std::iter_swap does not require that its arguments be of the same iterator type. Consequently, using std::iter_swap with iterators of different types will result in a compile error with this "only correct implementation", as far as I can tell.Quote:
Originally Posted by CornedBee
EDIT:
As far as I can tell it is not the fault of the MinGW port of g++ 3.4.5. There is nothing that says that std::swap must be used to implemented std::random_shuffle, so the only reasonable assumption is that the type must be copyable. Clearly, with a private copy constructor, the type in question is (at least partially) non-copyable.Quote:
Originally Posted by anon
The C++0x draft specifies the behaviour of iter_swap as "swap(*it1, *it2)", so this is definitely intended. You can always overload swap with different argument types, if the iter_swap implementation is correct and allows ADL of the swap implementation.
Also, until recently, iter_swap required that the value types of the two iterators be the same.
If that is the case then GCC 4.4 does not implement std::iter_swap correctly (unless std::swap is overloaded to allow swapping of objects of different types), and in fact the "both wrong" implementations implement it correctly since they implement it exactly as the specification (or at least the Microsoft version does; the MinGW port of g++'s implementation apparently just implements the swap manually, according to anon's quote).Quote:
Originally Posted by CornedBee
Yes, but the implementation that you claim to be the "only correct implementation" requires that the iterator types be the same (unless std::swap is overloaded to allow swapping of objects of different types), and apparently the C++0x draft does not make such a requirement.Quote:
Originally Posted by CornedBee
Eep, that was a typo. I forgot to dereference the iterators.
My point is this:
Code:namespace mine
{
class AlsoMine {
// Complicated.
void swap(AlsoMine& rhs) {
// Fast, exception-safe swap.
}
};
void swap(AlsoMine& lhs, AlsoMine& rhs) {
lhs.swap(rhs);
}
}
mine::AlsoMine *p1 = get_one(), *p2 = get_another();
std::iter_swap(p1, p2); // If this doesn't call AlsoMine::swap, the iter_swap implementation is broken.
Ah yes, that makes more sense.
Another idea besides giving out card indexes instead of cards, is for your card object to contain a member variable speficying which deck it came from. The constructor of a deck gets it's ID from a unique cookie generator, and each card in that deck gets that same ID. Then to give a card back, you pass it to your deckManager, which finds the right deck to put it into.
Just another idea.
I'd possibly go with the "a card is its cookie" approach too.
Perhaps this idea isn't what you were looking for. Every deck of cards is of a certain size. For example regular playing deck is 52 cards, 54 with jokers. Basically you create a single array containing all of the card objects for a single deck. Your actual deck would just be an array of integers that you would shuffle. For more then one deck you would just increase the size of the integer array. For example two decks would be an integer array containig numbers from 0 to 103 in sequential order. Just shuffle the integer array to shuffle your deck. Now that you have numbers ranging from 0-103 you would find which deck (0th 1st 2nd etc)the card belonged to by dividing the integer by 52 using integer division. To determine the actual card you would display you just use the size of a single deck (52) and take the modulus of the integer The resulting remainder should correspond to the the correct index into your array of card objects.