Help using destructors

[gmeza]

Hi everyone!

I am a hobbyist programmer. I used to be half decent in C, BASIC and some assembler (mostly PIC). After years without touching an IDE, I have recently brushed up on my C skills (some online courses), and I've started learning C++. Classes are my new challenge :-)

I am trying to fool create a linked list, and all goes well until the destructor is called, and then the program enters an endless loop as soon as I try to reference any element of the class. Here is the relevant code:
----------------------------------------------------------
class Person
{
private:
char Name[255];
char Address[255];
char *Notes;
int NoteSize;


public:
Person(); // Constructor
~Person(); //Destructor
void SetName (char*);
void SetAddress(char*);
int SetNotes(char*,int);
void GetName (char*);
void GetAddress(char*);
void GetNotes(char*);
int GetNotesSize();
Person *Successor;
};

typedef Person* PersonPtr;

Person::Person(void)
{
Notes = nullptr;
Successor = nullptr;
NoteSize = 0;
}

Person::~Person(void)
{
cout << "Destructor."<< endl;
if (Notes != nullptr) {
cout << "Freeing memory"<<endl;
delete [] Notes;
NoteSize = 0;
} else cout << "Notes empty"<<endl;
}

int Person::SetNotes(char *Note, int nsize)
{
int i;

try {
Notes = new char[nsize];
} catch (bad_alloc & ba) {
cout << stderr << "Can't allocate memory " << ba.what() <<endl;
NoteSize = 0;
return -1;
}

for(i=0;i<nsize;i++) *(Notes+i) = *(Note + i);
NoteSize = nsize;
return 0;
}

/* Other stuff ommitted for brevity */

void cleanup(PersonPtr Headnode)
{
PersonPtr Currentnode = Headnode, Nextnode;

while (Currentnode != nullptr) {
Nextnode = Currentnode->Successor;
delete [] Currentnode;
Currentnode = Nextnode;
}
}
--------------------------------------------------

I can create nodes, manipulate their data, create "Notes" on the nodes, etc, no issues. However, when "delete [] Currentnode" is executed, things go south. The "Destructor" line is output, but the moment we get to the "if" or I try to print the node name or anything, it dies.

What am I doing wrong?

[helios]

delete Currentnode;, not delete[] Currentnode;. You only delete[] pointers you allocated with new[].

By the way, don't do this:

try {
    Notes = new char[nsize];
} catch (bad_alloc & ba) {
    cout << stderr << "Can't allocate memory " << ba.what() <<endl;
    NoteSize = 0;
    return -1;
}

If you can't do anything with the exception just don't bother with a try-catch and let someone else handle it. It rarely makes sense to try to recover from an std::bad_alloc anyway.

[JRManx]

gmeza,

PLEASE USE CODE TAGS (the <> formatting button to the right), when posting code!

Along with the proper indenting, it makes it easier to read your code, and thus also easier to respond to your post.

Tutorials on how to use code tags:

http://www.cplusplus.com/articles/jEywvCM9/
http://www.cplusplus.com/articles/z13hAqkS/

I've found the second link to be the most help.

Hint: You can hit "edit post", highlight your code and then press the <> formatting button.
Note: This will not automatically indent your code! That part is up to you!

I've found it's easiest to copy and paste pre-indented code directly from the IDE, that way it is already properly formatted.

You can use the "preview" button at the bottom to see how it looks.

Using Code Tags, @Handy Andy, from cplusplus.com

First of all, have you run it through a debugger? That might help.

Here's a linked list header/program that I wrote. Try comparing the two and see what's different and what you might have done wrong.
Driver: https://pastebin.com/rbh90u31
Header: https://pastebin.com/k96Ce274

I'll make a driver for this and run it through my debugger and see what I get.

Oh, before I forget; what is the stuff that you "omitted for brevity?" I'll need to see it.

After actually looking at your code, yeah, I'll definitely need it. All of the SetName, SetAddress, and all that stuff.

Hope this helps!

[Edit] Do what @helios says– he knows what he's talking about, much more than I do.

[thmm]

@Helios

It rarely makes sense to try to recover from an std::bad_alloc anyway.

Why, I have seen a few times a dialog box with "There is not enough memory for this operation."
Closing Firefox gave me enough free memory to proceed with the other program.

[thmm]

@OP,
C++ has 2 linked lists already - std::list and std::forward_list.
No need to create another one.
I would suggest to learn about std::string and other basics first.

[deleted account xyzzy]

I've started learning C++.

Learn C++ is a good online tutorial for learning the basics of C++, including classes.
https://www.learncpp.com/

[ne555]

> /* Other stuff ommitted for brevity */
Don't do that, post enough code to reproduce your issue.
¿how are you building your list?

> Classes are my new challenge :-)
> I am trying to fool create a linked list
you've created a class Person with useless getters/setters that are badly implemented and that also represents a node.
¿where's your list class?
¿what do you expend so much time on the `data' class? there is no big difference between a list of Person, a list of strings or a list of integers
focus on the list operations: clear, traverse, add/remove element in front, back or middle, copy, etc.

[gmeza]

Thanks a lot, everyone!

This is program is an exercise, part of an online course I am following (on the LinkedIn learning platform). It's not the most useful piece of software (for example, exception catching was a must for the exercise, even if I am not doing anything particularly useful with it). std::list and so on is part of the next chapter in the course.

Sorry for the formatting, I copied the code directly from the IDE and assumed it would keep the indenting, which it didn't. I'll be more vigilant next time.

I am taking notes of all your comments, and will implement them (either on the code or in my next posts).

As for the debugger, I tried using the debugging capabilities of the IDE (Code::Blocks in my case, that is what the course called for), and I couldn't spot the issue since things went wrong over a single instruction, the pointers seemed to be correct, etc. (I didn't try to go and see into the assembler code to figure out what's wrong - my level of x86 assembly is somewhere between bad and absolutely useless).

For now, the solution from @helios fixed it. The brackets in the delete [] Currentnode; were the culprit (my guess, from what I understood now, is that code was trying to delete a vector of nodes when there is only one that is referenced, and thus entered an infinite loop).

Again, thanks a lot everyone, I've learned some useful stuff!

[seeplus]

I've made some changes for consideration. However without all the code etc not much testing can be done. You should use std::string rather than c-style char arrays.

class Person {
private:
	char Name[255] {};
	char Address[255] {};
	char* Notes {};
	int NoteSize {};

public:
	Person(); // Constructor
	~Person(); //Destructor
	void SetName(const char*);
	void SetAddress(const char*);
	int SetNotes(const char*, size_t);
	char* GetName() const;
	char* GetAddress() const;
	char* GetNotes() const;
	int GetNotesSize() const;
	Person* Successor {};
};

using PersonPtr = Person*;

Person::Person() {}

Person::~Person(void) { delete [] Notes; }

int Person::SetNotes(const char* Note, size_t nsize) {
	delete[] Notes;

	try {
		Notes = new char[nsize];
	}
	catch (bad_alloc& ba) {
		cerr  << "Can't allocate memory " << ba.what() << '\n';
		NoteSize = 0;
		Notes = nullptr;
		return -1;
	}

	snprintf(Notes, nsize, "%s", Note);
	NoteSize = nsize;
	return 0;
}

/* Other stuff ommitted for brevity */

void cleanup(PersonPtr& Headnode)
{
	while (Headnode) {
		const auto Nextnode {Headnode->Successor};

		delete Headnode;
		Headnode = Nextnode;
	}
}