I am writing a sale compiler for a garage sale my debate team is managing, this is the secondary compiler to the main class and it is giving me a runtime error. The visual studio code anyalsis says
1 2 3 4 5 6 7 8
C6385 Read overrun Reading invalid data from 'files[l].list': the readable size is '((unsigned int)=len*1)*80+4' bytes, but '160' bytes may be read. Sales Compiler salecompiler.cpp 23
'l' is NULL 17
Enter this loop, (assume 'l<len') 17
'l' is an Output from 'std::basic_string<char,std::char_traits<char>,std::allocator<char> >::=' (declared at c:\program files (x86)\microsoft visual studio 11.0\vc\include\xstring:969) 19
Enter this loop, (assume 'i<temp[l].length') 21
'i' may equal 1 21
Continue this loop, (assume 'i<temp[l].length') 21
Invalid read from 'files[l].list[1]', (readable range is 0 to 0) 23
Objects with pointers use the -> operator to access public methods instead of the . operator. So, any time you need to access the temp variable, you'll have to use it like this:
You also might want to make sure that the files variable is a pointer-type too, otherwise, you can't allocate space on the heap for a new SaleList object.
1 2 3
// Lines 3 and 4
int final = NULL;
SaleList* final = new SaleList[len];
You are stepping out of bounds of your array and accessing array elements that don't exist.
This block of code has several problems. Note my comments:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
SaleCompiler::SaleCompiler(SaleList * temp, int len )
{
final = NULL;
files = new SaleList[len]; // allocating 'len' elements for 'files'. This is good.
int l = 0;
for(l = 0;l<len;l++) // using 'l' to index 'files'. As long as 'l' is less than 'len', this is good.
{
files[l].fileName = temp[l].fileName;
files->list = new Sale[len]; // PROBLEM: you are doing files->list which is the same as files[0].list, which probably isn't what you wanted.
// also you are allocating 'len' elements and not 'temp[l].length' elements like you probably wanted
for(int i = 0;i<temp[l].length;i++)
{
files[l].list[i].setHour(temp[l].list[i].getHour()); // PROBLEM surfaces again here. You are using 'l' to index temp and files, which is fine, but you are using 'i' to index list, which is wrong.
// what if 'i' is greater than len? If that happens: EXPLODE
// also, because you never set files[l].list to anything, it's probably a bad pointer (see files->list problem above)
Another problem is you are doing all these new[]s but never any delete[]s, which means you are leaking memory. Every new[] must have a matching delete[].
The easiest way to manage that is to simply don't use new if you can avoid it.
Here, you could do this with a vector instead.
Futhermore... if you're using vectors, you don't have to worry about managing memory or even copying objects like this. You can just copy the vectors in full.
#pragma once
#include "Sale.h"
#include "SaleList.h"
#include <vector> // use vectors!
class SaleCompiler
{
public:
SaleCompiler(); // <- not important... but get rid of that (void) business. That's C garbage.
SaleCompiler::SaleCompiler(const std::vector<SaleList>& temp); // <-take a vector instead of a pointer+length
~SaleCompiler(); // <- getting rid of (void) garbage here too
void getFinal(Sale *temp);
void sortFinal();
void mergeFiles();
int getTotalSize()
{
return totalSize; // <- I'm not sure what to change this to. Either files.size() or final.size(), whichever size you intended here
}
protected:
/* SaleList * files; get rid of all this crap
Sale * final;
int totalSize;
int length;
*/
// replace it with vectors:
std::vector<SaleList> files;
std::vector<Sale> final;
};
/*
Now your big complicated constructor:
SaleCompiler::SaleCompiler(SaleList * temp, int len )
{
final = NULL;
files = new SaleList[len];
int l = 0;
for(l = 0;l<len;l++)
{
files[l].fileName = temp[l].fileName;
files->list = new Sale[len];
for(int i = 0;i<temp[l].length;i++)
{
files[l].list[i].setHour(temp[l].list[i].getHour());
files[l].list[i].setMin(temp[l].list[i].getMin());
files[l].list[i].setSec(temp[l].list[i].getSec());
files[l].list[i].setName(temp[l].list[i].getName());
files[l].list[i].setTime(temp[l].list[i].getTime());
files[l].list[i].setPrice(temp[l].list[i].getPrice());
}
files[l].length = temp[l].length;
}
length = len;
}
... can be greatly simplified:
*/
SaleCompiler::SaleCompiler(const std::vector<SaleList>& temp )
: files(temp) // done!
{
// files = temp; // <- this would have the same effect, more or less, and would also work
// but is less optimal that using files' constructor as illustrated above.
}
EDIT:
In case you are unfamiliar with vectors, they're basically resizable arrays:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
vector<int> foo(10); // an array of 10 ints
// can treat it just like an array:
foo[0] = 5;
foo[1] = foo[0] + 2;
// but you can also resize it:
foo.resize(50); // now it has 50 ints instead of just 10.
// and you can also append things to the end easily:
foo.push_back(5); // pushes '5' onto the end of the array. Array now has 51 elements.
// and they're also copyable:
vector<int> bar; // another array
bar = foo; // now the 'bar' array is a copy of the 'foo' array.
