Call Constructor from Copy Constructor -> Stack Overflow

[gedamial]

Look at this:

GString::GString(const char* CstyleString)
{
	cout << "CTOR (" << CstyleString << ")" << endl;

	if (CstyleString == nullptr)
		CstyleString = "";

	size = strlen(CstyleString);

	// +1 for the NULL character at the end (/0)
	mainString = new char[size + 1];

	for (int i = 0; i < size; i++)
		mainString[i] = CstyleString[i];

	// Now 'size' is the last element, that must be the NULL character
	mainString[size] = '\0';

	cout << "CONSTRUCED (" << mainString << ")" << endl << endl;
}

GString::GString(const GString& copy) : GString(copy)
{
	cout << "COPY CTOR" << endl;
}

###############

int main()
{
   GString my = "hello";
   GString my2(my);
   return 0;
}

the output is:

CTOR (hello)
CONSTRUCTED (hello)

STACK OVERFLOW EXCEPTION!

Why?

NOTE: There's a conversion from GString to char* which is totally fine and handled by me

		GString::operator const char*() const
		{
			return mainString;
		}

[JLBorges]

> Call Constructor from Copy Constructor -> Stack Overflow

Call Copy Constructor from Copy Constructor -> infinite recursion -> Stack Overflow


> There's a conversion from GString to char* which is totally fine and handled by me

GString::GString( const GString& copy ) : GString( static_cast<const char*>(copy) ) {}

[gedamial]

Why is it an infinite recursion?

p.s: the conversion i was talking about is now included in the post #1

[JLBorges]

> Why is it an infinite recursion?

Because it delegates to itself, without end.
GString::GString(const GString& copy) : GString(copy) // delegates to GString::GString( const GString& )

[gedamial]

Okay, thanks.

By the way, do you think this delegation between copy-default constructor is well designed?

[JLBorges]

> do you think this delegation between copy-default constructor is well designed?

Yes, if this is an academic endeavour; there is less code to read and maintain.

It is not the most efficient way of copying GString (size is recomputed).

[gedamial]

During the development of the class I've had the need to make another separate function which would have constructed my GString.

For example, I have made a function which deletes all the occurrences of a given character from the beginning and the end of a string.

Example:

......hello........ ---> hello

This function has to overwrite the already existing string. So I should have done something like:

delete[] mainString;
mainString = result;

To avoid this repetitive process, I thought to make a separated function like this:

void GString::Create(const char* source)
{
	if(mainString)
		delete[] mainString;		

	if (source == nullptr)
		source = "";

	size = strlen(source);

	// +1 for the NULL character at the end (/0)
	mainString = new char[size + 1];

	for (int i = 0; i < size; i++)
		mainString[i] = source[i];

	// Now 'size' is the last element, that must be the NULL character
	mainString[size] = '\0';
}

So that I could just have written:

Create(result);

So I decided to use this function also in my default GString constructor.

GString::GString(const char* CstyleString)
{
	Create(CstyleString);

}

Is it alright? What about you?

[JLBorges]

> I have made a function which deletes all the occurrences of a given character from
> the beginning and the end of a string. This function has to overwrite the already existing string.
> So I should have done something like:

> delete[] mainString;
> mainString = result;

The size of the trimmed string would not be greater than the size of the original; the buffer can be reused.

Something like this:

GString& GString::trim( char c )
{
    char* left = mainString ;
    while( *left == c ) ++left ;

    char* right = mainString + size - 1 ;
    while( right > left && *right == c ) --right ;

    std::move( left, right+1, mainString ) ;
    size = right - left + 1 ;
    mainString[size] = 0 ;

    return *this ;
}

http://coliru.stacked-crooked.com/a/28147e014b871d8a

[gedamial]

Thanks for the suggestion, that was useful.

However, that algorithm, as you pointed out, can save us from deleting the entire string.

However, this was a lucky situation.

But when it comes to other kind of algorithms, is that technique of making a separate Create() function so that it can check whether the string is a new or an existing one any good?

[JLBorges]

> is that technique of making a separate Create() function
> so that it can check whether the string is a new or an existing one any good?

Create is a bad name for this; something like assign would be more appropriate.
Assuming that an overloaded assignment operator is correctly implemented, this is what it would do.

In most situations, making the string larger would involve dynamically allocating a larger buffer and then replacing the existing buffer with the newly allocated buffer. For instance:

GString& operator+= ( const GString& that )
{
    char* temp = new char[ size + that.size + 1 ] ;
    std::strcpy( temp, mainString ) ;
    std::strcat( temp, that.mainString ) ;

    delete[] mainString ; // discard the current buffer
    mainString = temp ; // and use temp instead
    size += that.size ;

    return *this ;
}

[gedamial]

Assuming that an overloaded assignment operator is correctly implemented, this is what it would do

In fact, here is my assignment operator:

GString& GString::operator=(const GString& other)
{
	Create(other);
	return *this;
}

I think there's no problem here.

I also have a Clear function which is used both by the Create function and the Destructor

void GString::Clear()
{
	if (mainString)
        {
		delete[] mainString;
		size = 0;	 
         }				
}

Personally, I believe this is fine.

[gedamial]

One problem I have is this, for example:

GString GString::SubString(const int beginIndex, const int endIndex) const
{
	// The last "+1" is for the NULL character (\0) 
	char* tmp = new char[(endIndex - beginIndex) + 1 + 1];

	for (int i = beginIndex, j = 0; i <= endIndex; i++, j++)
		tmp[j] = mainString[i];

	tmp[(endIndex - beginIndex) + 1] = '\0';
			
	return GString(tmp);
}

###########

int main()
{
  	GString my2 = "Hello,I,Am,Michael";

	my2.SubString(0, 5);

        return 0;
}

When I execute, Visual Studio tells me about a Heap corruption.

BUT HOW?

The return statement calls the constructor which calls the Create() function ( see above ) which again calls the Clear() function (always above).

It checks whether it's a nullptr or not, but still it's crashing.

What's happening?

#######################################

I figured it out.


The return statement is constructing an object and hence, the mainString pointer wasn't inizialited to nullptr.

So, in the Clear() function, the check

void GString::Clear()
{
	if (mainString)
        {
		delete[] mainString;
		size = 0;	 
         }				
}

was always true, resulting in a dangerous delete[] operation on an undefined pointer/piece of memory.

Damn it.

[cire]

So, in the Clear() function, the check was always true, resulting in a dangerous delete[] operation on an undefined pointer/piece of memory.

It's kind of a pointless "check" anyway. delete handles nullptr pragmatically.

[JLBorges]

GString GString::SubString(const int beginIndex, const int endIndex) const
{
        // TO DO: *** validate that beginIndex and endIndex are within range***

	// The last "+1" is for the NULL character (\0) 
	char* tmp = new char[(endIndex - beginIndex) + 1 + 1]; // *** this memory is never released

	for (int i = beginIndex, j = 0; i <= endIndex; i++, j++)
		tmp[j] = mainString[i];

	tmp[(endIndex - beginIndex) + 1] = '\0';
			
	return GString(tmp);
}

[gedamial]

Uhm... I call the GString constructor passing the tmp as parameter.

I can't free that memory neither before the return (otherwise it will return an invalid string) nor after.

Umh... I think I'm forced to hard-code this:

GString GString::SubString(const int beginIndex, const int endIndex) const
{
	// IF NOT-IN-RANGE, return empty string
	if (!(beginIndex >= 0 && beginIndex < size    &&    endIndex >= 0 && endIndex < size))
		return GString();

	GString result;

	// The last "+1" is for the NULL character (\0) 
	int totalSize = (endIndex - beginIndex) + 1 + 1;
	char* tmp = new char[totalSize];

	for (int i = beginIndex, j = 0; i <= endIndex; i++, j++)
		tmp[j] = mainString[i];

	tmp[totalSize - 1] = '\0';

        // this calls the CONSTRUCTOR. whut? shouldn't it call the Assignment Operator?
	result = tmp;

         // free the old block
	delete[] tmp;

        // this calls the MOVE CONSTRUCTOR (see below)
	return result;
}

Move Constructor

GString::GString(GString&& move)
{
	size = move.size;
	mainString = new char[size];

	int i = 0;
	while (i < size)
	{
		mainString[i] = move.mainString[i];
		i++;
	}

	mainString[i] = '\0';
}

int main()
{
    Gstring h = "hello!";

    cout << h.SubString(3,5);
}

I end up with this: http://prntscr.com/ba4aeo

What is happening?

[gedamial]

Also, this code:

int main()
{	
	GString h = "hello";
	char* c = "ciao";

	h = c;
}

calls the Move Assignment Operator and then crashes in the Destructor

What is going on? Can you help me to figure it out?

GString.h - http://pastebin.com/yBZpNcif
GString.cpp - http://pastebin.com/VPSqRPbQ

[JLBorges]

> I call the GString constructor passing the tmp as parameter.
> I can't free that memory neither before the return (otherwise it will return an invalid string) nor after.

Yes.
"In most situations, making the string larger would should involve dynamically allocating a larger buffer and then replacing the existing buffer with the newly allocated buffer."
http://www.cplusplus.com/forum/general/191860/#msg925317

A few more constructors would help simplify the code. For instance:

struct GString
{
    char* buffer ;
    std::size_t size ;

    explicit GString( std::size_t sz = 0, char c = ' ' ) : buffer( new char[sz+1] ), size(sz)
    { std::fill_n( buffer, sz, c ) ; buffer[sz] = 0 ; }

    GString( const char* cstr ) : GString( cstr ? std::strlen(cstr) : 0 )
    { if(cstr) std::strcpy( buffer, cstr ) ; }

    GString( const char* begin, const char* end ) : GString(end-begin)
    { std::copy( begin, end, buffer ) ; }

    GString substr( std::size_t from_pos, std::size_t to_pos ) const
    {
        if( to_pos > size || to_pos < from_pos ) return GString() ;
        else return GString( buffer+from_pos, buffer+to_pos+1 ) ;
    }

    GString twice() const // academic: only for exposition
    {
        GString result( size*2 ) ;
        std::copy( buffer, buffer+size, result.buffer ) ;
        std::copy( buffer, buffer+size, result.buffer+size ) ;
        return result ;
    }

    // ...
};

> // this calls the CONSTRUCTOR. whut? shouldn't it call the Assignment Operator?
> result = tmp;

If there is no overloaded assignment operator which accepts a const char*, a temporary object would be constructed


> // this calls the MOVE CONSTRUCTOR (see below)
> return result;

Copy-elision (NRVO) may be (typically would be) applied.

[gedamial]

result = tmp;

The only assignment operator I have takes a GString object, while tmp is a char*.

I don't understand this part:

If there is no overloaded assignment operator which accepts a const char*, a temporary object would be constructed

You mean... the compiler transforms that line of code from

result = tmp;

to

result = GString(tmp);

If no, then what?

If yes, is there a solid rule for this to happen? Is there any documentation?

Thanks and sorry if I'm too harassing!

[JLBorges]

> The only assignment operator I have takes a GString object, while tmp is a char*
> You mean... the compiler transforms that line of code from result = tmp; to result = GString(tmp);
> is there a solid rule for this to happen? Is there any documentation?

Converting constructor: http://en.cppreference.com/w/cpp/language/converting_constructor
"a converting constructor specifies an implicit conversion from the types of its arguments to the type of its class."

[gedamial]

Okay thanks, but there's something weird to me:

Wasn't the object ALREADY constructed in this line

GString result;

calling the first constructor?

Why is the object constructed AGAIN here:

result = tmp;

?

We are constructing the object twice?!

By looking at this: http://stackoverflow.com/questions/19057311/why-constructor-is-being-called-twice

I'm pretty sure I understand what's going on:

The constructor gets called to construct ANOTHER TEMPORARY OBJECT using tmp as parameter of the first constructor which accepts a char*.

Then, a copy gets performed from the TEMPORARY to the result variable