Thread: std::swap( _Tp&, _Tp& ) calling copy constructor

  1. #16
    Use this: dudeomanodude's Avatar
    Join Date
    Jan 2008
    Location
    Hampton, VA
    Posts
    391
    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.
    Okay, I'm with you on that one.

    As a side note, the following compiles with VC++ 2005, but not with MinGW 3.4.5.
    So, basically I can't extend std::swap if I plan on keeping copying private?

    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.
    Ubuntu Desktop
    GCC/G++
    Geany (for quick projects)
    Anjuta (for larger things)

  2. #17
    Use this: dudeomanodude's Avatar
    Join Date
    Jan 2008
    Location
    Hampton, VA
    Posts
    391
    I'm still getting the privacy issues even after implementing the member swap to swap around members:
    Code:
    void Foo::m_swap( Foo& f ){
      int temp_value = this->m_value;
      this->m_value = f.m_value;
      f.m_value = temp_value;
    }
    What am I doing wrong that I still get the "blah is private within this context" error?

    EDIT: the error is still citing "Foo::Foo( const Foo& )" as the issue? I thought I was avoiding copying now? I'm lost.
    Last edited by dudeomanodude; 12-13-2008 at 10:38 AM.
    Ubuntu Desktop
    GCC/G++
    Geany (for quick projects)
    Anjuta (for larger things)

  3. #18
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by dudeomanodude
    So, basically I can't extend std::swap if I plan on keeping copying private?
    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
    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.
    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.)

    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.

    Quote Originally Posted by dudeomanodude
    What am I doing wrong that I still get the "blah is private within this context" error?

    EDIT: the error is still citing "Foo::Foo( const Foo& )" as the issue? I thought I was avoiding copying now? I'm lost.
    Post the smallest and simplest code that demonstrates the error.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  4. #19
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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)?

    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.
    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.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  5. #20
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    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.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  6. #21
    Use this: dudeomanodude's Avatar
    Join Date
    Jan 2008
    Location
    Hampton, VA
    Posts
    391
    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.
    Ubuntu Desktop
    GCC/G++
    Geany (for quick projects)
    Anjuta (for larger things)

  7. #22
    The larch
    Join Date
    May 2006
    Posts
    3,573
    Well, MinGW implements it like this:

    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;
        }
    Whereas MSVC uses std::swap:

    Code:
    template<class _FwdIt1,
    	class _FwdIt2> inline
    	void iter_swap(_FwdIt1 _Left, _FwdIt2 _Right)
    	{	// swap *_Left and *_Right
    	std::swap(*_Left, *_Right);
    	}
    I'm really tempted to modify MinGW's stl_algobase.h since it doesn't even attempt to use swap...
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  8. #23
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Oh god, they're both wrong. The only correct implementation, that won't break code, is
    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.
    }
    GCC 4.4 implements this correctly.
    Last edited by CornedBee; 12-13-2008 at 12:24 PM.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  9. #24
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by CornedBee
    The only correct implementation, that won't break code, is
    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.

    EDIT:
    Quote Originally Posted by anon
    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.
    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.
    Last edited by laserlight; 12-13-2008 at 12:14 PM.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #25
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    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.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  11. #26
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by CornedBee
    The C++0x draft specifies the behaviour of iter_swap as "swap(*it1, *it2)", so this is definitely intended.
    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
    Also, until recently, iter_swap required that the value types of the two iterators be the same.
    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 Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  12. #27
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    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.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  13. #28
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Ah yes, that makes more sense.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  14. #29
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    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.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  15. #30
    Registered User manofsteel972's Avatar
    Join Date
    Mar 2004
    Posts
    317
    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.
    Last edited by manofsteel972; 12-14-2008 at 03:56 AM.
    "Knowledge is proud that she knows so much; Wisdom is humble that she knows no more."
    -- Cowper

    Operating Systems=Slackware Linux 9.1,Windows 98/Xp
    Compilers=gcc 3.2.3, Visual C++ 6.0, DevC++(Mingw)

    You may teach a person from now until doom's day, but that person will only know what he learns himself.

    Now I know what doesn't work.

    A problem is understood by solving it, not by pondering it.

    For a bit of humor check out xkcd web comic http://xkcd.com/235/

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. pointer conversion problems with a copy constructor
    By stanlvw in forum C++ Programming
    Replies: 8
    Last Post: 01-14-2008, 12:06 AM
  2. Copy constructor for Queue ADT (as an Array)
    By clegs in forum C++ Programming
    Replies: 2
    Last Post: 11-28-2007, 11:05 PM
  3. Copy Constructor
    By noobcpp in forum C++ Programming
    Replies: 3
    Last Post: 07-01-2007, 06:29 AM
  4. Replies: 2
    Last Post: 04-04-2007, 06:34 PM
  5. calling default instead of argument constructor, why?!
    By cppn00b in forum C++ Programming
    Replies: 6
    Last Post: 01-30-2005, 04:24 AM