Re: Unitialized memory help
Please re-edit your post to use proper code tags, Your code is unreadable without them.
Regards,
Paul McKenzie
Re: Unitialized memory help
Code:
#define booklist "booklist.txt"
I don't recommend doing this. If you need a constant, it's better to do:
Code:
const string booklist = "boostlist.txt";
because that makes the constant substitution happen at compile time rather than preprocessor time, which means if there is a problem, you'll probably get a more useful error message.
Now, to your actual problem....
Code:
void grow()
{
int n=bookCount;
book* index2=new book[n+1];
for (int i=0;i<n;i++)
{
index2[i]=index[i];
}
delete[] index;
book* index=new book[n+1];
initarray(n+1);
for (int i=0;i<n;i++)
{
index[i]=index2[i];
}
bookCount=n+1;
index[bookCount].bc=bookCount;
delete[] index2;
}
There are actually several "problems" with this function, although only one is going to actual crash things. The others are efficiency issues.
First and foremost, this line looks very suspicious:
Code:
index[bookCount].bc=bookCount;
I suspect that's an out-of-bounce array access. Probably what's crashing you.
Almost as important, this function calls new[] three times, but delete[] only twice. Of course only two calls to new[] are made in this immediate code----actually only one is needed, we'll get to that momentarily----but you also call initarray(), which itself calls new[]. Therefore, you have a memory leak.
Now, to simple efficiency. In order to make an array larger, you have to:
1) Create a new, larger array.
2) Copy or move existing elements from the old array to the new one.
3) Delete the old array.
What you're actually doing is:
1) Create a new, larger array.
2) Copy all existing elements from the old array to the new one.
3) Delete the old array.
4) Create a second larger array.
5) Copy all the elements to the third array.
6) Delete the second array.
There's some extra copying going on here which isn't needed. After the "delete[] index;" line, only one more statement is needed to finish the logic (not including logic specific to your problem; I'm talking about array copying in general). Most of the second half of the function as-implemented is unnecessary.
Notice I said "copy or move" above. This probably won't be relevant to your class, but just FYI, C++0x has just introduced "move semantics". They're something you ought to know about even if the compiler used in your class is too old to support them; I suggest reading up.
Another efficiency point: each time the array is grown, you resize it only one element larger. From a heap manager perspective, this is actually a worst-case scenario, and you definitely can do better. What you need to do is add another variable to your class, bookCapacity, which is the actual size of the array as allocated. While bookCount would represent the logical size of the array, you would have no need to do an actual reallocation unless bookCount == bookCapacity when you try to grow(). In that case, typically what is done is you double bookCapacity, reallocate, and then you will again not have to reallocate for several more grow() calls.
Final observation is regarding your use of init() functions. Really, you ought to be doing all the init() logic in constructors, or at least make the init() function private and only call it from constructors; this would make things a lot easier, since other classes would never need to wonder whether they need to init() an object of your class before using it. As it happens, though, book::init() doesn't actually do anything that the default book constructor isn't doing anyway, so it isn't even necessary unless you want to clear and re-use an already existing book object.
Re: Unitialized memory help
Quote:
No error-checking required, although I'll probably add it if I have time.
Therein lies the rub -- to check for errors using coding like this will make your current code even larger and more hard to maintain and debug. If you learnbed proper container classes, this extra code to check is minimized.
Quote:
This compiles, but crashes
Compiling correctly has no bearng on whether the program will run correctly. Compiling successfully just means you have no syntax errors -- it doesn't tell you whether the program will work the way you want it to.
The first thing is that you use the wrong form of "delete". If you use "new[]", you must use "delete[]", not "delete". That's the first thing you need to fix.
Second, you could have coded a general dynamic array class yourself, and use that instead. Then you isolate the problem to the dynamic array class, and not have to micromanage this array inside of your main application.
Third, you posted a lot of other code that has absolutely nothing to do with adding a book to a dynamic array. You should just have a single function to add a book, and call it in a simple main() program. That is how you were (are) to approach writing such a program -- you don't approach this by writing the whole thing first. Once you are convinced that adding a book works with some dummy data, then you add it to the main app, and not before.
Last, have you used a debugger to debug the program? If not, I suggest you learn to use the debugger that comes with your compiler.
Regards,
Paul McKenzie
Re: Unitialized memory help
<quote>
There's some extra copying going on here which isn't needed.</quote>
How do you avoid the extra copying? Define books.index as a pointer to something like "index1", and when the index grows change the pointer to "index2" and delete index1?
<quote>First and foremost, this line looks very suspicious:</quote>
It actually crashes when I try to assign values to the new book in the index array. It gets past the grow() function alright. Sometimes it crashes assigning the ISBN, sometimes the Author's Last name, sometimes the Author's First name...it's like it doesn't have enough memory assigned to it.
Thanks for the help!
Re: Unitialized memory help
Quote:
Originally Posted by
jdblack
<quote>
There's some extra copying going on here which isn't needed.</quote>
How do you avoid the extra copying? Define books.index as a pointer to something like "index1", and when the index grows change the pointer to "index2" and delete index1?
In the middle of the function, you've got a filled array of the new size pointed to by index2. You've already deleted the array pointed to by index, so....all you really need to do is tell index to point at the array you've already got. That's it.
Just because you happened to point the local index2 at the new array when you allocated it doesn't mean that can't become the class's internal array later on with a bit of pointer assignment.
Quote:
It actually crashes when I try to assign values to the new book in the index array. It gets past the grow() function alright. Sometimes it crashes assigning the ISBN, sometimes the Author's Last name, sometimes the Author's First name...it's like it doesn't have enough memory assigned to it.
When it crashes doesn't always tell the whole story. All you know is that something got corrupted at some point prior to the crash. If you're lucky the problem will be right at the crash location, but that isn't always the case.
Re: Unitialized memory help
Quote:
Originally Posted by
jdblack
<quote>
These forums use square ([]) brackets, not angle brackets (look at your "<quote>"). You do the same thing with the code tags, you replaced the square brackets with the angle brackets.
Second, you didn't address fixing the usage of the wrong form of "delete" in your code.
Regards,
Paul McKenzie
Re: Unitialized memory help
Code:
book* index=new book[n+1];
This is not initializing the class's index variable, this is creating a new array which is local to the function. You want:
Code:
index = new book[n + 1];
or
Code:
this -> index = new book[n + 1]
These are identical, the latter is more specific and easier for other people to read, but the former is more common.
White space is ignored by the compiler and does not slow down compilation, and makes everything much easier to read. It's a good habit to get into.
Re: Unitialized memory help
Quote:
Originally Posted by
ninja9578
You want:
Good catch on the local, but that's not actually what he wants. The allocation should first be assigned to a local, then the copies made, and only then should the new array be assigned back into the object.
Re: Unitialized memory help
Oh, I didn't really look at the context, I just breezed through it due to lack of formatting. My mistake.
Re: Unitialized memory help
Quote:
Originally Posted by
Paul McKenzie
The first thing is that you use the wrong form of "delete". If you use "new[]", you must use "delete[]", not "delete". That's the first thing you need to fix.
Fixed, although this wasn't in any code currently being executed. I've only tried using the grow function right now, and it used "delete[]".
There's so much to respond to that I don't know where to start. :P
Quote:
First and foremost, this line looks very suspicious:
Code:
index[bookCount].bc=bookCount;
I took it out and replaced it with a static reference to the correct value, and nothing changed in the crash behavior.
Quote:
In the middle of the function, you've got a filled array of the new size pointed to by index2. You've already deleted the array pointed to by index, so....all you really need to do is tell index to point at the array you've already got.
How do I point index to a new object? I think I know how to do it for a normal variable, but I don't know the syntax for a constructor of a class.
Quote:
This is not initializing the class's index variable, this is creating a new array which is local to the function. You want:
Code:
index = new book[n + 1];
Changed it.
Quote:
Second, you could have coded a general dynamic array class yourself, and use that instead.
Point taken, but I don't feel up to starting over at this point.
Quote:
Last, have you used a debugger to debug the program?
Yes, and the output wasn't meaningful enough to me. This is the line it breaks on -
" static void __CLRCALL_OR_CDECL assign(_Elem& _Left, const _Elem& _Right)
{ // assign an element
_Left = _Right;
}"
Here's the current code after the recommended tweaks, same behavior.
Code:
#include <iostream>
#include <string>
#include <fstream>
using namespace std;
#define booklist "booklist.txt"
class book
{
public:
// Member functions
// Constructors
string isbn; // ISBN code
string bc; // Book code
string aln; // Author's last name
string afn; // Author's first name
string bt; // Book title
string y; // Year of publication
string price; // Price
// Display all fields.
void display()
{
cout << "Book code:\t" << bc << endl;
cout << "ISBN Number:\t" << isbn << endl;
cout << "Author's Last Name:\t" << aln << endl;
cout << "Author's First Name:\t" << afn << endl;
cout << "Book Title:\t" << bt << endl;
cout << "Year of publication:\t" << y << endl;
cout << "Price:\t\t" << price << endl << endl;
}
// Set all variables to empty values
void init()
{
aln.clear();
afn.clear();
bt.clear();
y.clear();
price.clear();
bc.clear();
}
};
class books
{
public:
// Member Functions
// Constructors
book* index;
int bookCount;
// Initialize the catalog as an array of size n
void initarray(int n)
{
index = new book[n];
bookCount = n;
}
// Displays all records
void displayAll()
{
for (int i=0;i<bookCount;i++)
{
index[i].display();
}
}
// Grow the catalog's array
void grow()
{
int& n=bookCount;
book* index2=new book[n+1];
for (int i=0;i<n;i++)
{
index2[i]=index[i];
}
delete[] index;
index = new book[n + 1];
for (int i=0;i<n;i++)
{
index[i]=index2[i];
}
bookCount=n+1;
index[bookCount].bc=bookCount;
delete[] index2;
}
// Shrink the catalog's array, removing the record of number n
void del(int d)
{
int n=bookCount-1;
book* index2=new book[n];
int j=0;
for (int i=0;i<n;i++)
{
if (n==d)
{
j=1;
}
index2[i]=index[i+j];
}
delete[]index;
book* index = new book[n];
for (int i=0;i<n-1;i++)
{
index[i]=index2[i];
}
bookCount=n-1;
delete[] index2;
}
};
void intro(); // Prototype declaration
int getOption(); // Prototype declaration
void loadfile(books&);
void addBook(books&);
string popTabdelim(string);
int main()
{
books catalog;
bool q=false;
loadfile(catalog);
intro();
do
{
int o=getOption();
switch (o)
{
case 1:
{
addBook(catalog);
break;
}
case 2:
{
break;
}
case 3:
{
break;
}
case 4:
{
catalog.displayAll();
break;
}
case 5:
{
break;
}
case 6:
{
//savecatalog();
q=true;
break;
}
}
}
while (!q);
return 0;
}
void intro()
{
cout << "\nWelcome to Jason Black's Book Cataloging system!\n\n";
}
int getOption()
{
int o; //Stores the user's choice.
cout << "Available options:\n";
cout << "\t1. Add a new book" << endl;
cout << "\t2. Find a book" << endl;
cout << "\t3. Delete a book" << endl;
cout << "\t4. Display all books" << endl;
cout << "\t5. Save changes" << endl;
cout << "\t6. Quit" << endl << endl;
cout << "Option:\t";
cin >> o;
cout << endl;
return o;
}
void loadfile(books& catalog)
{
string line;
int n=0;
ifstream bookfile;
bookfile.open(booklist);
while (!bookfile.eof())
{
getline(bookfile,line);
n++;
}
catalog.initarray(n);
catalog.bookCount=n;
bookfile.close();
bookfile.open(booklist);
for (int i=0;i<n;i++)
{
getline(bookfile,line,'\n');
catalog.index[i].bc=line.substr(0,line.find('\t'));
line=popTabdelim(line);
catalog.index[i].isbn=line.substr(0,line.find('\t'));
line=popTabdelim(line);
catalog.index[i].aln=line.substr(0,line.find('\t'));
line=popTabdelim(line);
catalog.index[i].afn=line.substr(0,line.find('\t'));
line=popTabdelim(line);
catalog.index[i].bt=line.substr(0,line.find('\t'));
line=popTabdelim(line);
catalog.index[i].y=line.substr(0,line.find('\t'));
line=popTabdelim(line);
catalog.index[i].price=line.substr(0,line.find('\t'));
}
}
string popTabdelim(string line)
{
int tabloc=line.find('\t');
string popped=line.substr(tabloc+1,line.length()-tabloc);
return popped;
}
void addBook(books& catalog)
{
catalog.grow();
book& b = catalog.index[catalog.bookCount];
cout << "Adding a new book.\n";
cout << "ISBN Number:\t";
cin >> b.isbn;
cout << "Author's Last Name:\t";
cin >> b.aln;
cout << "Author's First Name:\t";
cin >> b.afn;
cout << "Book Title:\t";
cin >> b.bt;
cout << "Year of Publication:\t";
cin >> b.y;
cout << "Price:\t\t";
cin >> b.price;
}
Re: Unitialized memory help
I bypassed the error by reconstructing the books object rather than attempting to change the book array inside the books object.
Thanks for the help everyone, got some good pointers for other less-critical errors.
Edit: And I just realized I never attached a booklist.txt...sorry about that! Probably would've helped if you all could've seen the error...
Re: Unitialized memory help
Quote:
Originally Posted by
jdblack
I took it out and replaced it with a static reference to the correct value, and nothing changed in the crash behavior.
Instead of that, do you know why that line is suspicious? That is how you solve problems -- you identify why the code causes problems before creating code that basically doesn't do anything. You can't just throw code at a problem and hope it works without knowing what is the exact problem.
The reason why that line doesn't work is that when you allocate n items, the items are indexed from 0 to n-1. You obviously have a memory overwrite on that line, since you allocated n items and are using n to reference an item, when items can only be referenced from 0 to n-1.
Given that, how do you solve it? You obviously have an indexing problem, where the index is going out of bounds. So you solve it by making sure you don't index items out of bounds.
The general problem with your code is writing too much at one time without testing each portion first. The "grow" function should have been fully tested prior to placing it in the middle of the program. Growing an array should be a basic, fundamental piece of the program that needs to be isolated.
Also, whether you're aware of it or not, C++ doesn't work the way you believe it does, where as soon as your code makes a mistake, you see the error at runtime. When you access invalid entries in an array, overwrite memory, etc. a C++ program need not crash immediately. It can silently keep running until a crash finally occurs, or it could run all the way without any visible problems. Unlike other computer languages, where you are immediately informed of any illegal accesses (i.e. Java, C#, etc.), C++ doesn't afford you this "luxury".
Code:
Yes, and the output wasn't meaningful enough to me. This is the line it breaks on -
" static void __CLRCALL_OR_CDECL assign(_Elem& _Left, const _Elem& _Right)
{ // assign an element
_Left = _Right;
}"
That line is part of the runtime, meaning your code did something illegal. This is what I mean -- that line of code is where your program finally breaks down -- it isn't where the problem originated.
Quote:
Here's the current code after the recommended tweaks, same behavior.
Again, have you used the debugger to debug your program? That is exactly what everyone here would need to do to debug your program, so you should be doing the same thing.
Regards,
Paul McKenzie
Re: Unitialized memory help
Quote:
Originally Posted by
jdblack
How do I point index to a new object? I think I know how to do it for a normal variable, but I don't know the syntax for a constructor of a class.
Observe:
Code:
// some time ago
int *arr = new int[5];
// now
int *arr2 = new int[10];
for (int i = 0; i < 5; i++)
arr2[i] = arr[i];
delete[] arr;
arr = arr2;
// some time later
delete[] arr;
Re: Unitialized memory help
Quote:
Originally Posted by
Lindley
Code:
for (int i = 0; i < 5; i++)
arr2[i] = arr[i];
Why not:
Code:
memmove(&arr2[0], &arr[0], 5 * sizeof(int));
Re: Unitialized memory help
Quote:
Originally Posted by
ninja9578
Why not:
Code:
memmove(&arr2[0], &arr[0], 5 * sizeof(int));
Well, for one thing, memcpy() is always at least as efficient as memmove(), and sometimes moreso. Furthermore, while both of those would be perfectly fine on ints, they would not generalize to any type; for instance, it could cause a crash if you tried it with an array of std::string.
So the "proper" solution if you didn't want to use a loop would be
Code:
std::copy(arr,arr+5,arr2);
This would of course decay to memcpy() internally when instantiated for ints.
Of course, that disregards move semantics. I'm not sure if there's a multi-element form of std::move on any compilers yet, or even if that's going to be the name for that function when there is.
Re: Unitialized memory help
Quote:
Originally Posted by
jdblack
Point taken, but I don't feel up to starting over at this point.
Bad choice. Using a std::vector instead of dynamic arrays will make your code much much simpler. It's not difficult to change the code either. Just change the type from pointer to std::vector<something>, compile and fix the errors one by one. Since accessing elements of the vector uses the same syntax as for a dynamic arrays, all you'll have to fix is the allocation/deallocation code, which is causing the problems anyhow.
Quote:
Originally Posted by
jdblack
Yes, and the output wasn't meaningful enough to me. This is the line it breaks on -
" static void __CLRCALL_OR_CDECL assign(_Elem& _Left, const _Elem& _Right)
{ // assign an element
_Left = _Right;
}"
It looks like you are using Visual Studio. If that's the case, open the 'Call stack' window when the debugger stops on this line and double click on the first line that is part of the code you've written. Then examine the expression that is pointed out (it may actually be one line above what is indicated).
Try searching for some online tutorials on how to debug a program in your compiler if you are not familiar with these techniques. It'll pay off, trust me. ;)
Edit: you can start with this FAQ
Re: Unitialized memory help
Quote:
Originally Posted by
Paul McKenzie
Instead of that, do you know why that line is suspicious? That is how you solve problems -- you identify why the code causes problems before creating code that basically doesn't do anything. You can't just throw code at a problem and hope it works without knowing what is the exact problem.
Sure you can, that's what I did and it's working now. :) I'll agree it's not ideal though because you don't learn from it.
Quote:
The reason why that line doesn't work is that when you allocate n items, the items are indexed from 0 to n-1. You obviously have a memory overwrite on that line, since you allocated n items and are using n to reference an item, when items can only be referenced from 0 to n-1.
Thanks, I should've caught that. I'll also make an effort to test each function first next time I right a complicated (for me) program. And thanks for the info about C++. I've used C# before quite a bit on my own.
Quote:
Bad choice. Using a std::vector instead of dynamic arrays will make your code much much simpler.
Dictated by my teacher unfortunately, no vectors or lists allowed.
Quote:
If that's the case, open the 'Call stack' window when the debugger stops on this line and double click on the first line that is part of the code you've written. Then examine the expression that is pointed out (it may actually be one line above what is indicated).
Sweet, thanks! I'm too busy now, but I'll do it in when I get home.
Re: Unitialized memory help
Quote:
Originally Posted by
jdblack
Dictated by my teacher unfortunately, no vectors or lists allowed.
So you're learning to write C++ programs as if it's the year 1990 instead of the year 2010. Instead of a book repository program, you had to write a dynamic array class disguised as a "real" assignment. If the course you're taking were honest, it should be called "C programming with some new syntax thrown in". It is not C++ programming, at least not in the way how the C++ language stands today, and truthfully, for the past 10+ years.
C++ has been standardized since 1998, and part of the standardization was to include containers into the language library. If the "no vector, no list" stuff keeps up, you won't be learning anything that a majority of the C++ programmers actually use.
If you were told up front to write a dynamic array class and only a dynamic array class, test it with various inputs, etc. that's one thing that could be acceptable (but for an intermediate programmer, not a beginner). You would then have used your home-made dynamic array class in your bookkeeping assignment, and see how it works. Personally, I have yet to see a beginner get a dynamic array class correct -- they are just not ready for such coding.
Regards,
Paul McKenzie
