deconstructor problems
In this code I generate three "dict" objects. When main returns the deconstructor ~dict() is called. But it's only one of the three objects that get runs through the deconstructor without failing. The second one generates a segmentation fault. Why doesn't this work for all three objects.
(Note: When i use the list member erase() instead of it's deconstructor the two first objects gets deallocated correctly, but the third seg faults).
# include <iostream>
# include <list>
# define NUMASC 128 /* # ASCII-values */
using namespace std;
template <class detype> class dictElem{
public:
detype data;
string key;
dictElem(string k, detype d){
key = k;
data = d;
}
};
template <class dtype> class dict{
public:
typedef dictElem<dtype> dictE;
typedef list <dictE> listOfDictElems;
typedef list <dictE> *arrayOfLists;
arrayOfLists a;
dict(){
a = new listOfDictElems[NUMASC];
}
void dictadd(string key, dtype data){
int idx = hash(key);
dtype *tmp;
if((tmp = dictIdx(key, idx)) != NULL){
*tmp = data;
return;
}
a[idx].push_back(*(new dictE (key , data)));
}
bool dictDel(string key){
int idx = hash(key);
typename listOfDictElems::iterator it;
for(it = a[idx].begin(); it != a[idx].end(); ++it)
if(it->key == key){
a[idx].erase(it);
return true;
}
return false;
}
dtype *dictIdx(string pipe, int idx = -1){
if(idx == -1)
idx = hash(pipe);
typename listOfDictElems::iterator it;// Ensure the compiler that listOf... is a typename
for(it = a[idx].begin(); it != a[idx].end(); ++it)
if(it->key == pipe)
return &(it->data);
return NULL;
}
void delDict(){
}
int hash(string base){
string::size_type len = base.length();
unsigned int i;
int val = 0;
for(i = 0; i < len; i++){
val += base[i];
}
return (val % NUMASC);
}
void printdict(){
int i, node;
cout << "[DICT START]\n";
for(i = 0; i < NUMASC; i++)
if(!a[i].empty()){
node = 0;
cout << "array[" << i << "]:\n";
typename listOfDictElems::const_iterator it;
for(it=a[i].begin(); it!=a[i].end(); ++it)
cout << "\tNode " << node++ << "\t[Key : " << it->key << "]\n\t\t[Data: " << it->data << "]\n";
}
cout << "[DICT END]\n";
}
dict<dtype> operator* (dict<dtype> b){
dict<dtype> ret;
int i;
typename listOfDictElems::iterator it;
for(i = 0; i < NUMASC; i++){
if(!b.a[i].empty())
for(it = b.a[i].begin(); it != b.a[i].end(); ++it)
ret.dictadd(it->key, it->data);
if(!a[i].empty())
for(it = a[i].begin(); it != a[i].end(); ++it)
ret.dictadd(it->key, it->data);
}
return ret;
}
dict<dtype> operator/ (dict<dtype> b){
dict<dtype> ret;
int i;
typename listOfDictElems::iterator it;
for(i = 0; i < NUMASC; i++)
if(!b.a[i].empty() && !a[i].empty())
for(it = a[i].begin(); it != a[i].end(); ++it)
if(b.dictIdx(it->key) != NULL)
ret.dictadd(it->key, it->data);
return ret;
}
dict<dtype> operator- (dict<dtype> b){
dict<dtype> ret;
int i;
typename listOfDictElems::iterator it;
for(i = 0; i < NUMASC; i++){
if(!b.a[i].empty() && !a[i].empty()){
for(it = a[i].begin(); it != a[i].end(); ++it)
if(b.dictIdx(it->key) != NULL)
continue;
else{
ret.dictadd(it->key, it->data);
}
}else{
for(it = a[i].begin(); it != a[i].end(); ++it)
ret.dictadd(it->key, it->data);
}
}
return ret;
}
~dict(){ //destructor
int i;
cout << "here\n";
for(i = 0; i < NUMASC; i++)
if(!a[i].empty())
a[i].~listOfDictElems();
delete [] a;
}
};
int main(){
typedef string type;
typedef dictElem<type> dictE;
typedef list <dictE> listOfDictElems;
typedef list <dictE> *arrayOfLists;
dict<type> d;
d.dictadd("key1", "string");
d.dictadd("kex2", "data");
d.dictadd("key2", "dd");
d.dictadd("key3", "test");
d.printdict();
// d.dictDel("key1");
d.printdict();
dict<type> e;
e.dictadd("kex2", "skalikkekomme");
e.dictadd("tt2", "data");
e.dictadd("tt3", "dd");
e.dictadd("tt4", "test");
e.printdict();
dict<type> p = d*e;
p.printdict();
p = d/e;
p.printdict();
p = d-e;
p.printdict();
// p.~dict();
}
[5299 byte] By [
tores] at [2007-11-18 19:17:29]

# 1 Re: deconstructor problems
copy constructor for class dict is missing.
when you have dynamic allocations then you must provide
a copy constructor,operator= and destructor.
some of your functions return dict<dtype> or get dict<dtype>
by value as a parameter, therefore the default copy constructor
is been invoked, a temporary object is created with a shallow copy of 'arrayOfLists a' which is pointer, then when the destructor
is called for the temporary object - you left with 'arrayOfLists a'
deleted...
btw, the following line im main, uses the copy constructor as well:
dict<type> p = d*e;
Guysl at 2007-11-9 0:32:22 >

# 2 Re: deconstructor problems
How should this copy constructor look like. I guess it's role is to decide how a "arrayOfLists" is to be copied?
tores at 2007-11-9 0:33:31 >

# 3 Re: deconstructor problems
Originally posted by tores
How should this copy constructor look like. I guess it's role is to decide how a "arrayOfLists" is to be copied? It's your class, you should know how a copy should be done. We can't help you there except that a copy constructor's prototype is as follows:
class foo
{
public:
foo(const foo& rhs); // copy constructor
foo& operator = (const foo& rhs); // assignment operator
};
The copy constructor is responsible for copying your objects from one object to another. There are plenty of examples here and in your favorite book on what a copy constructor and assignment op is supposed to look like, but again, it's your class -- you should know how to copy. Basically, your class must survive this very basic code:
int main()
{
YourClass a;
YourClass b = a; // copy
YourClass c;
c = a; // assignment
} // destruction
where "YourClass" is the class you are dealing with. The problem with what you wrote is that this very simple code will cause your class to fail miserably when you run it, in the copy, assignment, and destruction. The goal is to either code your classes to make sure copying, assignment, and destruction, are safe or make the copy and assignment unavailable by making the copy ctor and assignment operator private.
But to be truthful, I'm surprised you wrote the code you did and not be aware of the copy constructor and assignment operator being necessary, since it is one of the basics of proper C++ class coding. This is especially the case if you have members that point to dynamic memory. Any reason why you don't just use std::map instead of creating this very hard to maintain code for your "dictionary"? Is this a school assignment?
Regards,
Paul McKenzie
# 4 Re: deconstructor problems
Also, why not use this:
#include <list>
#include <vector>
template <class dtype> class dict{
public:
typedef dictElem<dtype> dictE;
typedef std::list <dictE> listOfDictElems;
typedef std::vector<std::list <dictE> > arrayOfLists;
arrayOfLists a;
dict(){
a.resize( NUMASC );
}
//...
Use std::vector. Now you don't worry about freeing the memory on destruction. Also, std::vector is copyable. If every member of your class is safely copyable, then you don't need to write an assignment operator or copy constructor, since the default copy/assignment is perfectly fine. The problem that you are having in your code is that you have a pointer to dynamically allocated memory. By eliminating it, you make your class copyable without any user-defined copy ctor/op =. Even the destructor is now empty since you now no longer need to say "delete [] a;".
The goal in a C++ program is to use new as little as possible. Veteran C++ programmers try to avoid operator new[] unless it is necessary.
Also, don't place "using namespace std;" in a header file (if you're doing that). Instead, prepend the variables that are in the std:: namespace with "std::".
Regards,
Paul McKenzie
# 5 Re: deconstructor problems
Yes it's a school assignment... I've never used c++ before, but have som experience with plain c. Started this assignment a few days ago, and have been searching the internet/ visited forums, and such whenever I was stuck. Haven't had a problem with the destructor until now (Because it was non-existant:)). Thanx to Paul Mckenzie for giving me the idea of using the list class. It made my life much easier:)
tores at 2007-11-9 0:36:26 >

# 6 Re: deconstructor problems
Also:
a[idx].push_back(*(new dictE (key , data)));
Why are you creating a new object? You don't need to do this. Just do this:
a[idx].push_back(dictE (key , data));
If you made the changes I outlined in this post and my previous post, then you shouldn't need a copy ctor or op = (or destructor). You no longer need to clean up the a array/vector, since you are no longer calling new to create a new dictE element.
Regards,
Paul McKenzie
# 7 Re: deconstructor problems
The following cleaned up version of your code does not suffer from the problems you were encountering:
#include <iostream>
#include <list>
#include <vector>
#include <string>
# define NUMASC 128 /* # ASCII-values */
using namespace std;
template <class detype>
class dictElem
{
public:
detype data;
std::string key;
dictElem(const std::string& k, detype d) : key(k), data(d)
{ }
};
template <class dtype> class dict
{
public:
typedef dictElem<dtype> dictE;
typedef std::list <dictE> listOfDictElems;
typedef std::vector<std::list <dictE> >arrayOfLists;
arrayOfLists a;
dict()
{
a.resize(NUMASC);
}
void dictadd(const std::string& key, dtype data)
{
int idx = hash(key);
dtype *tmp;
if((tmp = dictIdx(key, idx)) != NULL)
{
*tmp = data;
return;
}
a[idx].push_back(dictE (key , data));
}
bool dictDel(const std::string& key)
{
int idx = hash(key);
typename listOfDictElems::iterator it;
for(it = a[idx].begin(); it != a[idx].end(); ++it)
if (it->key == key)
{
a[idx].erase(it);
return true;
}
return false;
}
dtype *dictIdx(const std::string& pipe, int idx = -1){
if(idx == -1)
idx = hash(pipe);
typename std::list <dictE>::iterator it;
for(it = a[idx].begin(); it != a[idx].end(); ++it)
{
if (it->key == pipe)
return &(it->data);
}
return NULL;
}
void delDict(){
}
int hash(const std::string& base)
{
std::string::size_type len = base.length();
unsigned int i;
int val = 0;
for(i = 0; i < len; i++){
val += base[i];
}
return (val % NUMASC);
}
void printdict(){
int i, node;
cout << "[DICT START]\n";
for(i = 0; i < NUMASC; i++)
if(!a[i].empty()){
node = 0;
cout << "array[" << i << "]:\n";
typename listOfDictElems::const_iterator it;
cout << "[DICT END]\n";
}
}
dict<dtype> operator* (dict<dtype> b)
{
dict<dtype> ret;
int i;
typename listOfDictElems::iterator it;
for(i = 0; i < NUMASC; i++){
if(!b.a[i].empty())
for(it = b.a[i].begin(); it != b.a[i].end(); ++it)
ret.dictadd(it->key, it->data);
if(!a[i].empty())
for(it = a[i].begin(); it != a[i].end(); ++it)
ret.dictadd(it->key, it->data);
}
return ret;
}
dict<dtype> operator/ (dict<dtype> b){
dict<dtype> ret;
int i;
typename listOfDictElems::iterator it;
for(i = 0; i < NUMASC; i++)
if(!b.a[i].empty() && !a[i].empty())
for(it = a[i].begin(); it != a[i].end(); ++it)
if(b.dictIdx(it->key) != NULL)
ret.dictadd(it->key, it->data);
return ret;
}
dict<dtype> operator- (dict<dtype> b){
dict<dtype> ret;
int i;
typename listOfDictElems::iterator it;
for(i = 0; i < NUMASC; i++){
if(!b.a[i].empty() && !a[i].empty()){
for(it = a[i].begin(); it != a[i].end(); ++it)
if(b.dictIdx(it->key) != NULL)
continue;
else{
ret.dictadd(it->key, it->data);
}
}else{
for(it = a[i].begin(); it != a[i].end(); ++it)
ret.dictadd(it->key, it->data);
}
}
return ret;
}
};
int main(){
typedef string type;
typedef dictElem<type> dictE;
typedef list <dictE> listOfDictElems;
typedef list <dictE> *arrayOfLists;
dict<type> d;
d.dictadd("key1", "string");
d.dictadd("kex2", "data");
d.dictadd("key2", "dd");
d.dictadd("key3", "test");
d.printdict();
// d.dictDel("key1");
d.printdict();
dict<type> e;
e.dictadd("kex2", "skalikkekomme");
e.dictadd("tt2", "data");
e.dictadd("tt3", "dd");
e.dictadd("tt4", "test");
e.printdict();
dict<type> p = d*e;
p.printdict();
p = d/e;
p.printdict();
p = d-e;
p.printdict();
}
Note the usage of std::vector, and note how I pass the strings as const std::string references. Doing this makes sure that the compiler is making unecessary copies of your strings when you are passing them to functions that don't change these strings.
Also, there is no need for a destructor.
Regards,
Paul McKenzie
# 8 Re: deconstructor problems
One question... Why is it unwise to use "using namespace std" in the header-file?
tores at 2007-11-9 0:39:34 >

# 9 Re: deconstructor problems
Originally posted by tores
One question... Why is it unwise to use "using namespace std" in the header-file? Because you are bringing in the entire namespace into any module that includes your file. This will cause all sorts of problems if the CPP that includes the header has their own version of "vector", "list", etc. You never force a namespace onto the user of your header file.
Regards,
Paul McKenzie
# 10 Re: deconstructor problems
What's the difference of using "const std::string k" and "const std::string& k"?
I don't detect any difference when using the two.
tores at 2007-11-9 0:41:33 >

# 11 Re: deconstructor problems
A copy is still created by the compiler when you use "const std::string". You can't change the copy within the function (since it's const), but a copy is nevertheless created.
With a const std::string&, the copy is not made.
Regards,
Paul McKenzie
# 12 Re: deconstructor problems
Also, a small suggestion, consider changing the line:
#define NUMASC 128
to:
const int NUMASC = 128;
C++ programmers rarely define constants with #define.
cma at 2007-11-9 0:43:33 >
