map_problem

[Billy Patton]

With this class:
#ifndef __SGI_STL_MAP
#include <map>
#endif
#ifndef __IOSTREAM__
#include <iostream>
#endif

template <typename K,typename V> class HashTable: public std::map<K,V>
{
public:
bool Put(K& k,V& v,bool replace=false)
{
if (replace)
this->erase(k);
return (this->insert(std::make_pair(k,v))) ? true : false;
}
V& Value(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return iter->second;
return NULL;
}
bool Keys(K& k,bool firstp = false)
{
typedef typename std::map<K,V>::iterator map_iter;
static map_iter iter = this->begin();
if (iter == this->end()) return false;
k = iter->first();
++iter;
return true;
}
bool Exists(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return true;
return false;
}
void DumpKeys(void)
{
bool first = true;
K key;
while (this->Keys(key,first))
{
cout << " " << key << endl;
first = false;
}
}
unsigned int EntryCount(void) { return this->size(); }
};


I eliminated 300+ lines of .h and .cxx code
45 lines of code

Thanks all who helped

___ _ ____ ___ __ __
/ _ )(_) / /_ __ / _ \___ _/ /_/ /____ ___
/ _ / / / / // / / ___/ _ `/ __/ __/ _ \/ _ \
/____/_/_/_/\_, / /_/ \_,_/\__/\__/\___/_//_/
/___/
Texas Instruments ASIC Circuit Design Methodology Group
Dallas, Texas, 214-480-4455, (e-mail address removed)

 

[Bob Hairgrove]

[snip]

#ifndef __IOSTREAM__
#include <iostream>
#endif

Why? Isn't there already an include guard inside of <iostream>?
(Besides, if this is being compiled on MSVC, it won't help...you need
#pragma once).

template <typename K,typename V> class HashTable: public std::map<K,V>
{
public:
bool Put(K& k,V& v,bool replace=false)
{
if (replace)
this->erase(k);
return (this->insert(std::make_pair(k,v))) ? true : false;
}

No need for superfluous "this" everywhere ...

V& Value(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return iter->second;
return NULL;
}

The same...

bool Keys(K& k,bool firstp = false)
{
typedef typename std::map<K,V>::iterator map_iter;
static map_iter iter = this->begin();

Is this bound to work? "iter" is static, yet you depend on the object
being fully constructed to initialize it ...

if (iter == this->end()) return false;
k = iter->first();
++iter;
return true;
}

The argument firstp is never used...after X runs, where
X==this->size(), the function always returns false ... What is it you
are actually trying to do here??

bool Exists(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return true;
return false;
}

Why not just write:
bool Exists(K& k) { return find(k)!=end(); }

[snip...]

I eliminated 300+ lines of .h and .cxx code
45 lines of code

Looks like you aren't quite finished yet

 

[red floyd]

[redacted]
V& Value(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return iter->second;
return NULL;

confusing indentation. Also, NULL cannot be returned. I'm surprised
your compiler allowed it.

}
[redacted]

 

[Jonathan Mcdougall]

With this class:
#ifndef __SGI_STL_MAP
#include <map>
#endif
#ifndef __IOSTREAM__
#include <iostream>
#endif

The include guards are supposed to be in the headers so you don't need
to do that. [does the standard guarantees that?]

template <typename K,typename V> class HashTable: public std::map<K,V>

You are aware that you will not be able to use this class
polymorphically because you inherit from std::map which has no virtual
dtor? I would personally aggregate a std::map, but your design may
require inheritance.

{
public:
bool Put(K& k,V& v,bool replace=false)
{
if (replace)
this->erase(k);
return (this->insert(std::make_pair(k,v))) ? true : false;
}

insert() can't fail, except by throwing an exception.

V& Value(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return iter->second;
return NULL;

That's illegal. You cannot return null since you need a reference.
Throw an exception or change the return type to a pointer.

}
bool Keys(K& k,bool firstp = false)
{
typedef typename std::map<K,V>::iterator map_iter;
static map_iter iter = this->begin();
if (iter == this->end()) return false;
k = iter->first();
++iter;
return true;
}

Maybe the use of a custom iterator would be interesting here.


# include <iostream>
# include <map>

template <class K, class V>
class HashTable;

template <class K, class V>
class HashTable
{
private:
std::map<K, V> map_;

public:

template <class HTK, class HTV>
class key_iterator
{
private:
typedef HashTable<HTK, HTV> HT;

HT &ht_;
typename HT::iterator itor_;

public:
key_iterator(HashTable<HTK, HTV> &ht,
typename HashTable<HTK, HTV>::iterator itor);

K &operator*();
K *operator->();
K &operator++();
K operator++(int);

// ...
};


key_iterator<K, V> key_begin()
{
return key_iterator<K, V>(*this, map_.begin());
}

key_iterator<K, V> key_end()
{
return key_iterator<K, V>(*this, map_.end());
}
};

template <class K, class V>
void f(HashTable<K, V> &ht)
{
std::cout << "Here are the keys: " << std::endl;

for (typename HashTable<K, V>::key_iterator itor=ht.key_begin();
itor!=ht.key_end(); ++itor)
{
std::cout << *itor << std::endl;
}
}

I just wanted to show the iterator's definition, but I got carried

bool Exists(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return true;
return false;
}
void DumpKeys(void)

void in the argument list is considered bad style in C++ and is
redundant anyways.

{
bool first = true;
K key;
while (this->Keys(key,first))
{
cout << " " << key << endl;
first = false;
}
}
unsigned int EntryCount(void) { return this->size(); }

Prefer using std::size_t.

};

In general, it's very ok. Here's a couple of things:

1) I usually put a bunch of typedefs

typedef std::map<K, V> map_t;
typedef std::map<K, V>::iterator iterator;
typedef std::map<K, V>::const_iterator const_iterator;
...

at the top of the class so you won't need to do it in every function.

2) Using this-> is redundant in your case.


Jonathan

 

[Billy Patton]

[snip]

#ifndef __IOSTREAM__
#include <iostream>
#endif

Why? Isn't there already an include guard inside of <iostream>?

Compiling only on Linux and Solaris.
THere is already a guard, but it has to do a file io for it to find the gaurd.
This prevents the file io.

(Besides, if this is being compiled on MSVC, it won't help...you need
#pragma once).

template <typename K,typename V> class HashTable: public std::map<K,V>
{
public:
bool Put(K& k,V& v,bool replace=false)
{
if (replace)
this->erase(k);
return (this->insert(std::make_pair(k,v))) ? true : false;
}

No need for superfluous "this" everywhere ...

V& Value(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return iter->second;
return NULL;
}

The same...

bool Keys(K& k,bool firstp = false)
{
typedef typename std::map<K,V>::iterator map_iter;
static map_iter iter = this->begin();

Is this bound to work? "iter" is static, yet you depend on the object
being fully constructed to initialize it ...

if (iter == this->end()) return false;
k = iter->first();
++iter;
return true;
}

The argument firstp is never used...after X runs, where
X==this->size(), the function always returns false ... What is it you
are actually trying to do here??

bool Exists(K& k)
{
typedef typename std::map<K,V>::iterator map_iter;
map_iter iter = this->find(k);
if( iter != this->end() )
return true;
return false;
}

Why not just write:
bool Exists(K& k) { return find(k)!=end(); }

[snip...]

I eliminated 300+ lines of .h and .cxx code
45 lines of code

Looks like you aren't quite finished yet

___ _ ____ ___ __ __
/ _ )(_) / /_ __ / _ \___ _/ /_/ /____ ___
/ _ / / / / // / / ___/ _ `/ __/ __/ _ \/ _ \
/____/_/_/_/\_, / /_/ \_,_/\__/\__/\___/_//_/
/___/
Texas Instruments ASIC Circuit Design Methodology Group
Dallas, Texas, 214-480-4455, (e-mail address removed)

 

[msalters]

[snip]

template <typename K,typename V> class HashTable: public

{
public:
bool Put(K& k,V& v,bool replace=false)
{
if (replace)
this->erase(k);
return (this->insert(std::make_pair(k,v))) ? true : false;
}

No need for superfluous "this" everywhere ...

In fact, there is. Older compilers failed to do proper name
lookup, but new compilers correctly require it. The reason is
that tw-phase name lookup looks up "insert" in phase 1
(template compilation, non-dependent name) and "this->insert"
in phase 2 (instantiation, 'this' is dependent name).
In the first phase, the templated base class is unknown,
so std::map<...>::insert won't be found.

Regards,
Michiel Salters