C++

Forum

 
 
class with two std::string: memory allocation problem?

maeriden (872)
I have this class
1
2
3
4
5
6
7
8
9
10
 
class Line
{
    public:
        Line() {lineNum = 0;}
        ~Line() {};

        long lineNum;
        std::string speaker;
        std::string text;
}


When I debugged the code to see what was wrong it showed me that the sequence of character for both strings (member "_M_p") was stored at the same address in memory, therefore when I tried adding characters to one of them, the other was modified as well.
I worked around the problem by using string::reserve(), but I'm really puzzled by this behavior. Is there a "correct" way to solve this?

If it makes any difference, I'm using C::B 10.05 with MinGW, and in the code I instantiate an array of Line based on data retrieved from a text file
 
 
Line* lines = new Line[validLines];
Last edited on
vlad from moscow (6539)
I think that using the capital S in this declaration std::String text; is a typo.

As for your problem it can be said nothing because there is no enough code to conclude something reasonable.
maeriden (872)
Yes it was a typo, I corrected it

As for the code, the only line I think could be important is the one I already wrote, but here's the function where the memory is allocated
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
 
void Class::LoadFile(const char* fName)
{
	std::ifstream in(fName, std::ios_base::in|std::ios_base::binary);
	if(!in.is_open())
	{
		std::cout << fName << ": invalid file name" << std::endl;
                return;
	}

	int i = 0;
	char line[1000];
	int validLines = 0;

	while(in.good())
	{
		in.getline(line, 1000);
		for(i = 0; line[i] != '\0'; ++i)
			if(line[i] == ':' or line[i] == '"')
			{
				++validLines;
				break;
			}
	}

	if(validLines == 0)
	{
		std::cout << fName << " is not a valid file" << std::endl;
                return;
	}

        // lines is a member of Class
	lines = new Line[validLines];
	in.clear();
	in.seekg(0, std::ios_base::beg);


	int curLine = 0;
	int j = 0;

	while(in.good())
	{
		in.getline(line, 1000);
		for(i = 0; line[i] !='\0'; ++i)
		{
			switch(line[i])
			{
			case ':':
				++i;
				for(j = 0; line[i] != ':'; )
				{
					lines[curLine].speaker[j++] = line[i++];
				}
				break;

			case '"':
				++i;
				for(j = 0; line[i] != '"'; )
				{
					lines[curLine].text[j++] = line[i++];
				}
				lines[curLine].lineNum = curLine+1;
				++curLine;
				break;
			} // switch(line[i])
		} // for(i = 0; line[i] != EOL; ++i)
	} // while(in.good())
	in.close();
}


It simply counts the number of lines in a text file where certain characters appear (: and "), allocate exactly the memory needed to store all those lines and copy the contents of each line in one of the array elements

To get around the problem I changed the constructor of Line to be like this
1
2
3
4
5
6
 
Line::Line()
{
        lineNum = 0;
        speaker.reserve(32);
        text.reserve(128);
}


Please note that the code compiles without any problem, or i wouldn't have been able to debug it

Would this suffice?
Last edited on
vlad from moscow (6539)
Your code is incorrect. For example let consider the following code snip

1
2
3
4
5
6
7
 
			case ':':
				++i;
				for(j = 0; line[i] != ':'; )
				{
					lines[curLine].speaker[j++] = line[i++];
				}
				break;



You do not check that the end of the string was encountered

for(j = 0; line[i] != ':'; ) // what about line[i] == '\0' ?

Further you may not use speaker[j++] because the memory for the string speaker was not allocated. You should use lines[curLine].speaker += line[i++];

So you should update your proggram.
Last edited on
cire (8284)
Using operator[] with an index that is out of range for a string does not extend the string.

Neither of those loops should be looping on the in.good() condition.

1
2
3
4
5
6
7
 
			case ':':
				++i ;
				while (line[i] != ':' && line[i] != '\0' && i<1000 )
				{
					lines[curLine].speaker += line[i++];
				}
				break;


while(in.good())

should become:

while(in.getline(line,1000))


Some things you might consider. You allocate memory for lines here, but don't make any attempt to see if any memory was allocated previously (and if it was, you have an obvious memory leak here.) Using a string as opposed to an array of char for line.
Last edited on
maeriden (872)
You do not check that the end of the string was encountered

for(j = 0; line[i] != ':'; ) // what about line[i] == '\0' ?


Right, I forgot to add that part, thank you.

Further you may not use speaker[j++] because the memory for the string speaker was not allocated. You should use lines[curLine].speaker += line[i++];


Awesome, you got it fixed in a blink
Now the two strings have the same address until the first call to operator+=, which makes it change

Thank you very much
---------------------------------------------------------------
Neither of those loops should be looping on the in.good() condition.

Why? I can see that I save a call, but is there a specific reason for that?

Some things you might consider. You allocate memory for lines here, but don't make any attempt to see if any memory was allocated previously (and if it was, you have an obvious memory leak here.) Using a string as opposed to an array of char for line.

Well, actually when I encounter errors, like lines ending before a closing : or " is found, I delete[] the memory. It's not possible that other memory is allocated previously because this function is called only once
1
2
3
4
5
6
7
8
9
10
11
 
case '"':
	++i;
	while(line[i] !='"')
	{
		if(line[i] == '\0')
		{
			std::cout << "Error while scanning file: unexpected EOL at line " << curLine+1 << ", position " << i+1 << std::endl;
			delete[] lines;
			return;
		}
		script[curLine].text += line[i++];

I also changed the for to a while since j is not needed anymore
Last edited on
cire (8284)
Neither of those loops should be looping on the in.good() condition.

Why? I can see that I save a call, but is there a specific reason for that?


Yes. In order for in.good() to return false an input operation has to have failed.

That means an in.getline(line, 1000) failed and you processed line as if it didn't.


It's not possible that other memory is allocated previously because this function is called only once


That assumption makes memory leaks likely in the future. You're also not checking to see if i ever exceeds the bounds of line.

maeriden (872)
Yes. In order for in.good() to return false an input operation has to have failed.

That means an in.getline(line, 1000) failed and you processed line as if it didn't.


The C++ documentation about getline says that the eofbit flag is set when the end of the source of characters is reached, so the eofbit flag should be set right as the last in.getline() is called, making in.good() return false before a new call to in.getline() that would retrieve invalid data, right?.
My concern is that getline returns an std::istream&. Can that be used to check for truth?

That assumption makes memory leaks likely in the future.

I see. Do I just check if lines points to a valid location to do that?

You're also not checking to see if i ever exceeds the bounds of line.

Since getline already appends a '\0' at the end of the char array, and I'm checking for that, I thought it was safe. So I guess that if the line is longer than 1000 characters the '\0' is not automatically appended at the end of the array, right?
cire (8284)
The C++ documentation about getline says that the eofbit flag is set when the end of the source of characters is reached, so the eofbit flag should be set right as the last in.getline() is called, making in.good() return false before a new call to in.getline() that would retrieve invalid data, right?.

It might. It might not. It depends on the format of the file, and in particular the last "line" in the file. However, it isn't good to rely on it always being the way you expect when it's so easy to have it work for all cases without doing any extra work.

My concern is that getline returns an std::istream&. Can that be used to check for truth?

Of course.

Since getline already appends a '\0' at the end of the char array, and I'm checking for that, I thought it was safe. So I guess that if the line is longer than 1000 characters the '\0' is not automatically appended at the end of the array, right? 


1
2
3
4
 
				for(j = 0; line[i] != ':'; )
				{
					lines[curLine].speaker[j++] = line[i++];
				}


Where are you checking for '\0' again? And since you weren't checking the state of the input stream immediately after getline, you had no way of knowing whether there even was a '\0' anywhere in the array the first time through.




maeriden (872)

It might. It might not. It depends on the format of the file, and in particular the last "line" in the file. However, it isn't good to rely on it always being the way you expect when it's so easy to have it work for all cases without doing any extra work.

I see. Thanks for the tip. By the way it's not that I don't want to change it, just trying to understand better

Where are you checking for '\0' again?
1
2
3
4
5
6
7
8
9
10
11
 
case ':':
	++i;
	while(line[i] !=':')
	{
		if(line[i] == '\0')
		{
			std::cout << "Error while scanning file: unexpected EOL at line " << curLine+1 << ", position " << i+1 << std::endl;
			delete[] lines;
			return;
		}
		script[curLine].speaker+= line[i++];

When I posted the code I removed comments and some other print instructions, and I deleted that part by mistake too

And since you weren't checking the state of the input stream immediately after getline, you had no way of knowing whether there even was a '\0' anywhere in the array the first time through.

I'm sorry, but I don't understand what's the problem
Topic archived. No new replies allowed.