Constructors/Assignment operators for class with int pointer

[SirEnder125]

Hello! This is my current code:

#include <iostream>
#include <string>
#include <vector>
#include <fstream>
#include <filesystem>

namespace fs = std::filesystem;

class MyClass {
	int* m_data;
	int m_size;
public:
	// Default constructor. (1)
	MyClass() {
		// Is this constructor necessary/safe??
		std::cout << "Default constructor.\n";
		
		m_data = nullptr;
		m_size = 0;
	}
	
	// Default constructor. (2)
	MyClass(int size) {
		std::cout << "Default constructor.\n";

		m_data = new int[size];
		m_size = size;
	}

	// Copy constructor.
	MyClass(const MyClass& other) {
		std::cout << "Copy constructor.\n";

		m_data = new int[other.m_size];
		*m_data = *other.m_data;
		m_size = other.m_size;
	}

	// Assignment operator.
	MyClass& operator=(const MyClass& other) {
		std::cout << "Assignment operator.\n";

		if (this == &other) return *this;

		// Are the next two lines really necessary?
		delete[] m_data;

		m_data = new int[other.m_size];

		*m_data = *other.m_data;
		m_size = other.m_size;

		return *this;
	}

	// Move constructor.
	MyClass(MyClass&& other) {
		std::cout << "Move constructor.\n";

		m_data = new int[other.m_size];
		*m_data = *other.m_data;
		m_size = other.m_size;

		// I guess this is so that the address will not be freed when the temporary is destroyed. Correct?
		other.m_data = nullptr;
		other.m_size = 0;
	}

	// Move assignment operator.
	MyClass& operator=(MyClass&& other) {
		std::cout << "Move assignment operator.\n";

		delete[] m_data;

		m_data = new int[other.m_size];
		*m_data = *other.m_data;
		m_size = other.m_size;

		other.m_data = nullptr;
		other.m_size = 0;

		return *this;
	}

	~MyClass() {
		std::cout << "Destructor.\n";

		delete[] m_data;
	}

	int* data() {
		return m_data;
	}

	void setData(/*int* data*/) {
		// Not sure about what parameters to take here. ???
	}
};

int main(int argc, char **argv) {
	MyClass mc1(20);
	MyClass mc2(0);
	mc2 = mc1;

	return 0;
}

Basically I need help understanding if I wrote good, fast constructors/assignment operators that I need here. Also setData() is a bit confusing. I want to set that data in the array/pointer m_data. Do I treat it as an array? Do I just copy the address, or do I derefrence m_data and the parameter passed?

[dutch]

There's quite a few mistakes.
In your copy ctor (and elsewhere), this only copies a single integer:

        *m_data = *other.m_data;

In your move ctor, you still create a new array for m_data. That's not a move. A move is where you don't create a new array and instead "steal" the array of the other object.

[jonnin]

these days you generally assign in a list rather than the constructor for basic initial values, or just set them in the class.

https://en.cppreference.com/w/cpp/language/constructor

line 35 looks like an error to me.... you can't copy blocks of memory with an =

the assignment op, you can see if they are the same size or larger. that is, if the this* object's internal pointer's size is >= the one that is being copied into it, you don't need to delete and remake it. If it is smaller, you must. Its not wrong to always do it, just slow (and of course putting 3 items in a 5 billion sized block is wasteful, so you could do a sanity check on relative sizes).

yes, nulling out on the move prevents the shared pointer (hmm..!!) from being cleared out. Speaking of which, if you are open to studying more, a shared pointer would be better here!

apart from line 35 I didnt eyeball any big problems. There may be others...

set data... you can do any of many things. you could pass in a pointer, and the class could run something much like the move constructor over it. If you put a try/catch around it, they could even hand you an array and you could make it 'work' depending on how you define 'work'. You could pass in an int and an index (but, instead, you should overload the [] operator!). You could do something else. what do you want it to do??

[Ganado]

Speaking of which, if you are open to studying more, a shared pointer would be better here!

I don't think a shared pointer is it. I think each MyClass owns its own memory, so you'd just need a unique_ptr. But if we're going to use library features like that, we should just go all the way and use a std::vector. (Still a good thing to practice, to know how to use smart pointers for other situations, of course.)

[jonnin]

true, the move had me thinking shared, but its not necessary there.

[seeplus]

As a first refactor, consider:

#include <iostream>
#include <utility>
#include <algorithm>
#include <initializer_list>

class MyClass {
	int* m_data {};
	size_t m_size {};
	size_t m_used {};

public:
	// Default constructor
	MyClass() { }

	// Constructor
	MyClass(size_t size) : m_size(size), m_data(new int[size]) {}

	// Constructor - Initializer list
	MyClass(const std::initializer_list<int>& il) : m_size(il.size()), m_used(il.size()), m_data(new int[il.size()]) {
		std::copy(il.begin(), il.end(), m_data);
	}

	// Swap
	void swap(MyClass& rhs) noexcept {
		std::swap(rhs.m_data, m_data);
		std::swap(rhs.m_size, m_size);
		std::swap(rhs.m_used, m_used);
	}

	// Copy constructor.
	MyClass(const MyClass& rhs) : m_size(rhs.m_size), m_used(rhs.m_used), m_data(new int[rhs.m_size]) {
		std::copy_n(rhs.m_data, m_used, m_data);
	}

	// Assignment operator.
	MyClass& operator=(const MyClass& rhs) {
		auto tmp {rhs};

		swap(tmp);
		return *this;
	}

	// Move constructor.
	MyClass(MyClass&& other) noexcept {
		swap(other);
	}

	// Move assignment operator.
	MyClass& operator=(MyClass&& other) noexcept {
		swap(other);
		return *this;
	}

	~MyClass() {
		delete[] m_data;
	}

	void addData(const std::initializer_list<int>& il) noexcept {
		for (auto it = il.begin(); m_used < m_size && it != il.end(); ++it)
			m_data[m_used++] = *it;
	}

	[[nodiscard]]
	const int operator[](size_t ind) const {
		return m_data[ind];
	}

	[[nodiscard]]
	int& operator[](size_t ind) {
		return m_data[ind];
	}

	void display() const noexcept {
		for (size_t i = 0; i < m_used; ++i)
			std::cout << m_data[i] << ' ';

		std::cout << '\n';
	}
};

int main() {
	MyClass mc1(7);
	MyClass mc2;

	mc1.addData({1, 2, 3, 4, 5});
	mc1.addData({6, 7, 8, 9});

	mc2 = mc1;

	mc1.display();
	mc2.display();

	mc2[3] = 89;

	mc2.display();
}

1 2 3 4 5 6 7
1 2 3 4 5 6 7
1 2 3 89 5 6 7

But why? If you need something like this why not just use a vector?

[JLBorges]

Ideally, if a public member function swap is implemented, also make the class Swappable

Typical implementations either
1) Define a non-member swap in the enclosing namespace, which may forward to a member swap if access to non-public data members is required
2) Define a friend function in-class (this approach hides the class-specific swap from name lookup other than ADL)
https://en.cppreference.com/w/cpp/named_req/Swappable

Consider marking swap as noexcept

[seeplus]

Consider marking swap as noexcept

Correct. I overlooked that. Changed above. Thanks.

[SirEnder125]

But why? If you need something like this why not just use a vector?

@seeplus I am only making this class to learn about the different kinds of constructors and assignment operators.

[SirEnder125]

@seeplus

When I try to compile your code:

g++ -std=c++2a -fno-elide-constructors -O0 -g3 -Wall -c -fmessage-length=0 -o "Source\\main.o" "..\\Source\\main.cpp" 
..\Source\main.cpp: In constructor 'MyClass::MyClass(size_t)':
..\Source\main.cpp:58:9: warning: 'MyClass::m_size' will be initialized after [-Wreorder]
   58 |  size_t m_size {};
      |         ^~~~~~
..\Source\main.cpp:57:7: warning:   'int* MyClass::m_data' [-Wreorder]
   57 |  int* m_data {};
      |       ^~~~~~
..\Source\main.cpp:66:2: warning:   when initialized here [-Wreorder]
   66 |  MyClass(size_t size) : m_size(size), m_data(new int[size]) {}
      |  ^~~~~~~
..\Source\main.cpp: In constructor 'MyClass::MyClass(const std::initializer_list<int>&)':
..\Source\main.cpp:59:9: warning: 'MyClass::m_used' will be initialized after [-Wreorder]
   59 |  size_t m_used {};
      |         ^~~~~~
..\Source\main.cpp:57:7: warning:   'int* MyClass::m_data' [-Wreorder]
   57 |  int* m_data {};
      |       ^~~~~~
..\Source\main.cpp:69:2: warning:   when initialized here [-Wreorder]
   69 |  MyClass(const std::initializer_list<int>& il) : m_size(il.size()), m_used(il.size()), m_data(new int[il.size()]) {
      |  ^~~~~~~
..\Source\main.cpp: In copy constructor 'MyClass::MyClass(const MyClass&)':
..\Source\main.cpp:59:9: warning: 'MyClass::m_used' will be initialized after [-Wreorder]
   59 |  size_t m_used {};
      |         ^~~~~~
..\Source\main.cpp:57:7: warning:   'int* MyClass::m_data' [-Wreorder]
   57 |  int* m_data {};
      |       ^~~~~~
..\Source\main.cpp:81:2: warning:   when initialized here [-Wreorder]
   81 |  MyClass(const MyClass& rhs) : m_size(rhs.m_size), m_used(rhs.m_used), m_data(new int[rhs.m_size]) {
      |  ^~~~~~~
g++ "-LC:\\MinGW\\x86_64-w64-mingw32\\lib" -o Test.exe "Source\\main.o" -lws2_32 

15:43:07 Build Finished. 0 errors, 9 warnings. (took 3s.435ms)

EDIT: I think I did something wrong to the code, never mind.

[dutch]

Those are just reorder warnings telling you that your data members will be initialized in the order that they appear in the class definition, no matter what the order of the member initializers. Putting the member initializers in the same order as the data members will silence the warning.

[SirEnder125]

Alright, I changed the ordering. The warnings didn't really bother me much, but I got an error in my own copy of the code because I accidently made swap take an rvalue reference to a MyClass instead of a reference.

[seeplus]

Those are just reorder warnings telling you that your data members will be initialized in the order that they appear in the class definition, no matter what the order of the member initializers.

You don't get those warnings with VS2019, but I was careful to avoid using data members in the initialisations to avoid this issue. It can be a problem, though, if this issue isn't known about.