Copy and Move Constructor causes failure

[LukeProducts]

Hi everybody,

i've trained to write a std::string clone [whole code at the bottom]


Then, i wanted to use this class as a vector member. Ive written some code like the copy and move constructor, but it seems to be be incorrect, i do get overflows if i add elements to my std::vector<String> container.

Constructors meant:

    String(const String &cp) {
        std::cout << "copy constructor" << "\n";
        if (initialized)
            free(_value);
        _value = (char *) malloc(cp._len());
        _value = strdup(cp._value);
    }
    String (const String&& other) {
        std::cout << "move constructor" << "\n";
        if (initialized)
            free(_value);

        _value = (char *) malloc(other._len());

        _value = strdup(other._value);
    }

This part might be incorrect but i dont get why.

Do you know how to make my code work?


Code:

class String {

    char *_value;

    bool initialized = false;

    char* _substr(const char *src, size_t m, size_t n) {
        size_t len = n - m;
        
        char *dest = (char*)malloc(sizeof(char) * (len + 1));
        for (size_t i = m; i < n && (*(src + i) != '\0'); i++)
        {
            *dest = *(src + i);
            dest++;
        }

        *dest = '\0';

        return dest - len;
    }

    size_t _len() const {
        size_t res{};
        while (_value[res++]);
        return res - 1;
    }

public:

    String(const char *val) {
        _value = (char *) malloc(sizeof val / sizeof *val);
        if (_value == nullptr)
            throw std::runtime_error("error while allocating memory");
        strcpy(_value, val);
        initialized = true;
    }
    String(char *val) {
        _value = (char *) malloc(sizeof val / sizeof *val);
        if (_value == nullptr)
            throw std::runtime_error("error while allocating memory");
        strcpy(_value, (const char *) val);
        initialized = true;
    }
    String() {
        _value = (char *) malloc(sizeof(char));
        if (_value == nullptr)
            throw std::runtime_error("error while allocating memory");
        strcpy(_value, "");
        initialized = true;
    }


    // Copy - Constructor IMPORTANT

    String(const String &cp) {
        std::cout << "copy constructor" << "\n";
        if (initialized)
            free(_value);
        _value = (char *) malloc(cp._len());
        _value = strdup(cp._value);
    }
    String (const String&& other) {
        std::cout << "move constructor" << "\n";
        if (initialized)
            free(_value);

        _value = (char *) malloc(other._len());

        _value = strdup(other._value);
    }


    char &operator[](int index) const {
        if (index >= 0)
            return *(_value + index);
        
        return *(_value + (_len() - (index * -1)));
        
    }

    const char *c_str() const {return _value;}

    const unsigned long long length() const {return _len();};

    void push_back(char q);
    void pop_back();

    String &append(const char *);

    String &operator+=(const char *app_str) {
        return append(app_str);
    }

    String &insert(int, const char *);
    String substr(size_t begin, size_t end) {return String{_substr( _value, begin, end)};}

    long long find_first(char) const;
    std::vector<size_t> findall(char) const;
    std::vector<size_t> findall(const char *);
    long long find_first(const char *);
    long long find_last(char);
    long long find_last(const char *);

    std::vector<const char *> split(char);
    std::vector<const char *> split(const char *);



    ~String() {free(_value);}
    
}

Thanks in advance

Luke

[salem c]

Read your previous thread again.
https://www.cplusplus.com/forum/general/283787/

> _value = (char *) malloc(sizeof val / sizeof *val);
This is wrong (twice)
1. Use new[] in C++
2. The sizeof calculation is just plain wrong.
The space occupied by a C-style string is given by strlen(s)+1

[Peter87]

Make sure you allocate enough space to store the null character.

Don't use sizeof on char pointers. Instead use std::strlen to get the length of the string.

You should not need to have a variable named initialized. If there are situations when the _value pointer should not point to anything just set it to null. Passing null to free is safe and will do nothing so you don't need to check.

[keskiverto]

Why do you check initialization in constructors? The this is created there; how could it have been used "before"?

Why is the move constructor identical to copy constructor? Yes, it can be, but the idea of move is to be more efficient than copy.

[seeplus]

As a starter, consider the below which has copy/move constructors and operator():

#include <utility>
#include <iostream>
#include <cstring>

class String {
	char* _value { new char[1] {} };

	size_t _len() const {
		auto tmp { _value };

		for (; *tmp; ++tmp);
		return tmp - _value;
	}

public:
	~String() {
		delete[] _value;
	}

	String() {}
	String(const char* val) : _value(strcpy(new char[std::strlen(val) + 1], val)) {}
	String(const String& cp) : String(cp._value) {}

	String(String&& other) noexcept {
		std::swap(_value, other._value);
	}

	char operator[](int index) const {
		return index >= 0 ? *(_value + index) : *(_value + _len() + index);
	}

	char& operator[](int index) {
		return index >= 0 ? *(_value + index) : *(_value + _len() + index);
	}

	const char* c_str() const { return _value; }
	const size_t length() const { return _len(); };

	String& operator=(String rhs) {
		std::swap(_value, rhs._value);
		return *this;
	}
};

int main() {
	String s1;
	String s2 { "qwerty" };
	String s3(s2);

	std::cout << s1.c_str() << '\t' << s1.length() << '\n';
	std::cout << s2.c_str() << '\t' << s2.length() << '\n';
	std::cout << s3.c_str() << '\t' << s3.length() << '\n';

	String s4(std::move(s3));

	std::cout << s3.c_str() << '\t' << s3.length() << '\n';
	std::cout << s4.c_str() << '\t' << s4.length() << '\n';

	std::cout << s4[2] << '\n';
	std::cout << s4[-1] << '\n';

	s3 = s4;
	std::cout << s3.c_str() << '\t' << s3.length() << '\n';
}

        0
qwerty  6
qwerty  6
        0
qwerty  6
e
y
qwerty  6

[kbw]

Looking at your code:

    String(const String &cp) {
        std::cout << "copy constructor" << "\n";
        if (initialized)  // wrong test, we know we're not initialized
            free(_value);
        _value = (char *) malloc(cp._len() + 1); // either, malloc(stdlen(cp._value) + 1); strcpy(_value, cp._value);
        _value = strdup(cp._value); // or, strdup(cp._value), not both
    }
    String (const String&& other) {  // const is wrong, it shouldn't be there
        std::cout << "move constructor" << "\n";
        if (initialized)  // again, wrong, we know we're not initialized
            free(_value);

         _value = (char *) malloc(other._len());
        _value = strdup(other._value);

        // do the move from other to here
        _value = other._value;
        other._value = nullptr;
    }

[seeplus]

also move constructor shouldn't cause an exception in any cases and should be marked as noexcept.

copy constructor can also be coded in terms of constructor(const char*) so code isn't duplicated.

[kbw]

copy constructor can also be coded in terms of constructor(const char*) so code isn't duplicated.

Not quite.