Why is "while (*sea++ = *river++);" not working as it should?

Question

My college professor recently gave us a task of implementing our own smart pointer classes. In his boilerplate code for copying strings I found this piece beautiful syntactic sugar:

while (*sea++ = *river++);// C Sting copy 

I looked further into this code and found that it is the exact code as found in strcpy.c and further explanation of how it works, in the following stackoverflow question: How does “while(*s++ = *t++)” copy a string?

When I tried to use this syntactic sugar in my following code it gave garbage as a result and deleted the string stored in "river":

    #include<iostream>
    #include<cstring>

    using namespace std;
        void main()
        {
            const char *river = "water";// a 5 character string +  NULL terminator

            char *sea = new char[6];

            while (*sea++ = *river++);

            cout << "Sea contains: " << sea << endl;
            cout << "River contains: " << river << endl;
        }

RESULT:

I know that I can simply achieve the desired result with the following code instead:

    int i = 0;
    while (i<6)
    {
        sea[i] = river[i];
        i++;
    }

But this is not the answer I want. I would like to know is there something wrong with the implementation of my while loop or the instantiation of my char pointers?

Answer 1

You are getting garbage displayed because your pointers are pointing at garbage when you go to display them. You are advancing the pointers while looping, but you need to use the original pointers when displaying the data.

Also, you have a memory leak, as you are not freeing the char[] buffer.

Try this instead:

#include <iostream>
#include <cstring>

using namespace std;

int main()
{
    const char *river = "water";// a 5 character string +  NULL terminator

    char *sea = new char[6];

    const char *p_river = river;
    char *p_sea = sea;
    while (*p_sea++ = *p_river++);

    cout << "Sea contains: " << sea << endl;
    cout << "River contains: " << river << endl;

    delete [] sea;
    return 0;
}

Answer 2

The "error" is technically an undefined behavior due to pointer gone out of bound respect to to the memory they refer.

The interesting part is WHY that happens.

And it is due by the confusion between the pointers and what they point to.

sea and river are not strings. The strings are anonymous variable residing somewhere in memory, with the two pointer indicating their start.

You should never touch them, otherwise you will be no more able to access those strings further.

If you need to move pointers, use other ones.

A more proper example should be this one

 using namespace std;
 int main() //< note `void main` is a C++ dialcet
 {
     // note the use of `const` after the `*`: 
     // you cannot modify these pointers.
     const char * const river = "water";  // a 5 character string +  NULL terminator
     char * const sea = new char[6];

     {
          // in this inner scope, other non-const pointers are
          // initialized to point to the same memory
          const char* r = river;
          char* s = sea;
          while (*s++ = *r++); // the loop moves the mutable pointers
          // note how `river++` or `sea++` is an error, being them `*const`
     }

     // there are no more `r` and `s` here, but `sea` and `river` are still the same.
     cout << "Sea contains: " << sea << endl;
     cout << "River contains: " << river << endl;

     //now we have an array allocated with new to return to the system
     delete[] sea; //< the importance to not move the `sea` pointer
}

Note how delete deletes the array, not the pointer.

To go more in advance, two things can be done.

The first is make the inner scope a proper function:

 using namespace std;

 void copy(const char* r, char* s)
 {
     // in this function, other non-const pointer (parameters) are
     // initialized to point to the same memory upon call
     while (*s++ = *r++); // the loops moves the mutable pointers
     // note how `river++` or `sea++` is an error, being them not visible.
 }

 int main() //< note `void main` is a C++ dialect
 {
     const char * const river = "water";  // a 5 character string +  NULL terminator
     char * const sea = new char[6];

     copy(river, sea);         

     cout << "Sea contains: " << sea << endl;
     cout << "River contains: " << river << endl;

     //now we have an array allocated with new to return to the system
     delete[] sea; //< the importance to not move the `sea` pointer
}

and the second is get rid of the new/delete pair in a same context, using -for example- an std::unique_ptr<char[]>

But this is taking too far!

Answer 3

Because ASCII-art is always useful for figuring out pointer problems...

This is before your while loop:

 river points to
 |
 V
+-+-+-+-+-+--+
|w|a|t|e|r|\0|
+-+-+-+-+-+--+

+-+-+-+-+-+-+
|?|?|?|?|?|?|
+-+-+-+-+-+-+
 ^
 |
 sea points to

But after your loop, you have this:

              river points to
              |
              V
+-+-+-+-+-+--+
|w|a|t|e|r|\0|
+-+-+-+-+-+--+

+-+-+-+-+-+--+
|w|a|t|e|r|\0|
+-+-+-+-+-+--+
              ^
              |
              sea points to

So you see, the copy has indeed been performed, but you've also moved the pointers. That means if you try and print them out, you'll get whatever garbage lies after those memory blocks.

Were you to save copies of the pointers from before the while loop, and print those out instead, you'd get the output you're looking for.