Vector subscript out of range

Started by
8 comments, last by Zakwayda 14 years, 1 month ago
http://codepad.org/MrFVTlqQ I'm getting this weird error in a class I'm developing. First time I've ever used vectors but I haven't done anything strange. I have a vector called Library. It fills up with data fine. I can read the data off in TPdict::Train(), ::Push_Library and the constructor. But when I try to read it off from other functions such as TPdict::PWord() it crashes and I get an error saying: Debug Assertion Failed Expression: vector subscript out of range. Which implies that its empty because I am still reading from the same index I was before. I just discovered that if I call TPdict::PWord(); from the constructor it works fine. But if I call it from my main loop it crashes.
Advertisement
Quotation from the code you posted:
void TPdict::Push_Library(int Freq, std::string EntryWord){	LibBuffer.Freq = Freq;	LibBuffer.iLetter = new char[EntryWord.length()];	LibBuffer.iLetter = EntryWord.c_str();	Library.push_back(LibBuffer);	std::cout << Library[0].iLetter;		//DeleteArrayO(LibBuffer.iLetter); // I have no idea why this doesn't work}


Some questions:

1. What exactly does DeleteArrayO() do? Does it delete the memory associated with iLetter? If it does, then probably it isn't a good idea. vector::push_back() isn't smart enough to copy a C-style string allocated by new, so when you delete LibBuffer.iLetter it'll be deleted from the pointer that is now contained in the vector. The end result is that you have a vector full of null pointers.

2. Why are you using C-style strings in the first place? Using C++ strings would solve some of the problems that I can see. You're passing a std::string to the Push_Library method, why not just use std::strings for everything?

3. Why are you allocating memory, assigning a pointer to it, and then overwriting that pointer with another value? And why are you doing it without deleting the memory you just allocated? Not to mention that a few lines later it looks like you're trying to delete memory that's been given to you by std::string::c_str(), which is probably not a good idea since std::string handles that sort of thing for you.
LibBuffer.iLetter = new char[EntryWord.length()];LibBuffer.iLetter = EntryWord.c_str();


I can't see anything that obviously looks like it would throw a "subscript out of range" error. Would it be possible to post some more code, particularly the exact line that causes the assertion?
Quote:when I try to read it off from other functions such as TPdict::PWord() it crashes

It may be obvious, but have you created an instance of TPdict when you call yourInstance->PWord()?

If you add std::cout << "Library size " << Libary.size() << endl; to PWord(), do you, in fact, get zero?

EDIT: It shouldn't have anything to do with the problem, but I noticed the same things Oberon_Command mentioned. Changing the char* to string would be a good thing.

Please don't PM me with questions. Post them in the forums for everyone's benefit, and I can embarrass myself publicly.

You don't forget how to play when you grow old; you grow old when you forget how to play.

LibBuffer.iLetter = new char[EntryWord.length()];LibBuffer.iLetter = EntryWord.c_str();


This doesn't do anything remotely like what you think it does. (I can say that confidently because if you thought it did what it actually does, you would never think of writing it. :) )

Just make LibBuffer.iLetter a string, and assign it, which will make a copy and take care of all the memory management for you. You can index into a std::string just like you would into an array, so there's no reason you'd need the array. (Further, even if you were doing everything correctly here, what good do you think it could possibly do to deallocate the array immediately after setting up its contents? The structure would just be left with a dangling pointer.)

And WTF is it called "iLetter", anyway? Even if you thought you had a good reason to use Hungarian notation, you're aware that 'i' normally stands for 'integer', yeah? A char array-allocation isn't an integer and neither is a std::string instance.
Quote:Original post by Gegenki
I just discovered that if I call TPdict::PWord(); from the constructor it works fine. But if I call it from my main loop it crashes.

Despite all the nastiness, that should work:
int main(){  TPdict dict;  dict.PWord();}
Assuming you're using an IDE, have you tried setting breakpoints and watching Library's state?
Quote:Original post by Wan
Despite all the nastiness, that should work


Undefined behaviour is a pain, hmm? :) That broken memory management nonsense has got to go.
I got rid of DeleteArrayO and I changed everything to strings which made everything much easier.

I set up some extra break points and test variables and eventually fixed the problem but I don't know why it fixes the problem.

If I had a variable set in the constructor it would be 0 when I attempted to use it unless the variable was used before the constructor ended.
The start of my program which was actually using the class looked like this

#include <iostream>#include <conio.h>#include <stdio.h>#include <string>#include "TPdict.h"using namespace std;TPdict Lib1;int main(){}


Once I changed it to this as is what Wan said should work

#include <iostream>#include <conio.h>#include <stdio.h>#include <string>#include "TPdict.h"using namespace std;int main(){TPdict Lib1;}


It started working - but I don't understand why it was an issue. Its only recently that I've started to use local variables. Aside from the fact that classes are something I tend to still declare globally, If I needed another function to use the class I'd now have to pass it. What's wrong with initialising it globally?
Quote:Original post by Gegenki
It started working - but I don't understand why it was an issue. Its only recently that I've started to use local variables.


O_O

How in the hell did you manage to learn C++ - or programming in general - in a way that didn't show you like hundreds of examples of local variables before a global one?

That said, your problem description doesn't really make any sense. For one thing, "setting" a variable (assigning to it) is a way of "using" it. For another, you can't use an object that hasn't been finished being constructed yet.

Anyway, you certainly don't need a global variable to act as a "buffer" for creating instances in the Library vector. You don't need a variable at all! All you need is the ability to create the instance with an expression. In other words, a constructor for the struct.

// Now with improved variable names. Please think carefully about the differences.struct WordData{	int frequency;	std::string word;	// We add a constructor:	WordData(int frequency, const std::string& word): frequency(frequency), word(word) {}};// Instead of a global 'library', we add the vector as a data member of the// dictionary:// class Dictionary {// 	...// 	std::vector<WordData> words;// };Dictionary::Dictionary() {	// When you are inside a member function, you don't have to specify	// that you're calling another member function. That's the default:	// C++ will look for the function inside the class first, and then	// outside.	Train();}// Don't make your class write out data. That's not its responsibility. Have it// provide data, and let the calling code write it out. Also, if you're going// to have a return value, use a meaningful one.int Dictionary::FrequencyOfFirstWord(){	// You're aware, I hope, that array indices begin at 0 in C++.	// I used .at() here so that there is some error checking: you can't	// return the frequency of the first word if there are no words.	return words.at(0).frequency;}// You don't have a meaningful value to return here, and there is nothing that// would check the return value, so don't return a value.void Dictionary::Train(){	Push_Library(4, "Apple");	Push_Library(4, "Heal");	Push_Library(2, "Heap");	Push_Library(2, "Help");	Push_Library(5, "Hello");	Push_Library(1, "xylophone");	// I assume this part is just for debugging purposes. You can still	// use your helper function for it.	std::cout << FrequencyOfFirstWord();}void Dictionary::Push_Library(int frequency, const std::string& word){	// It's really this easy:	words.push_back(WordData(frequency, word));	// Look ma, no variable.}



Quote:Aside from the fact that classes are something I tend to still declare globally, If I needed another function to use the class I'd now have to pass it. What's wrong with initialising it globally?


What you apparently really mean is "what's wrong with having the function use a global variable instead of passing the data as a parameter?". The answer should be obvious: the parameters of a function represent its input. By using the global variable, you are (a) hiding that input and (b) removing flexibility, because you always use the global (instead of being able to pass in anything of the appropriate type, as a parameter).

Thanks, this is very useful information for the future. I've only just gotten a chance to look at the code again.

In the line
WordData(int frequency, const std::string& word): frequency(frequency), word(word) {}

I didn't realise you could use a constructor within a struct and I don't really understand how this line works. I tried looking it up but I don't know what I'm looking for.
When I search for information on structs and constructors online they don't seem to cover this type of thing - especially the stuff after the colon
Quote:When I search for information on structs and constructors online they don't seem to cover this type of thing - especially the stuff after the colon
Try searching for 'c++ initializer list'.

This topic is closed to new replies.

Advertisement