tags:

views:

895

answers:

4

Hi,

I'd have to say I'm no expert on using the STL. Here's my problem, I have a class Called LdapClientManager which maintains a number of LDAP clients that are managed by ID. The container holding the LdapClients is declared as a member variable i.e.

typedef std::map<int, LdapClient *> LdapClientMap;
LdapClientMap _ldapClientMap;

The following function fails to compile with the error:

LdapClient * LdapClientManager::getLdapClient(unsigned int templateID)
{
    // Do we have an LdapClient
    LdapClientMap::const_iterator it = _ldapClientMap.find(templateID);
    if (it == std::map::end) {
        // no existing client, lets create it
        LdapClient * ldapClient = new LdapClient();
        if (ldapClient == NULL) {
            // TODO: handle out of memory condition
        }

        _ldapClientMap[templateID] = ldapClient;
        return ldapClient;
    }

    return it->second;
}

Unfortunately I get the following error at compile time, what does it mean. I haven't found a solution in google as yet.

LdapClientManager.cc: In member function LdapClient* LdapClientManager::getLdapClient(unsigned int)': LdapClientManager.cc:33: template class std::map' used without template parameters

+6  A: 

Replace std::map::end with _ldapClientMap.end(). Also, new never returns 0, it throws an exception if the allocation fails.

Note that the program can be made much shorter.

LdapClient * LdapClientManager::getLdapClient(unsigned int templateID)
{
    LdapClient *& value = _ldapClientMap[templateID];
    if (value == 0)
        value = new LdapClient();
    return value;
}
avakar
typename before LdapClientMap? Why?!
EFraim
Whoops, removed. It would be necessary if `getLdapClient` were a template. Thanks, EFraim.
avakar
In std::map, operator[] is a mutating operation. This may limit the use (cannot be used inside a constant method), or it can affect the system (a later call to find() will yield an iterator into the array, the map will grow in memory for each failed test...)
David Rodríguez - dribeas
Also take into account, that even pods like the pointer, will not be initialized to 0, so your check when to create new client object might be wrong
CsTamas
driberas, the problem will only arise if the allocation fails (which happens rarely and will in most cases cause a program shutdown anyway).
avakar
CsTamas, `map`'s `operator[]` zero-initializes the value. The code works just fine.
avakar
Doh, seems obvious now... typical, I copied it from an example which was wrong. Thanks, didn't realize new throws an exception if allocation fials.
Matt H
Thanks for that. I've settled on storing the objects themselves rather than pointers and simply returning _ldapClientMap[templateID];for getLdapClient... even simpler!
Matt H
+1  A: 

It means exactly what it says it means. std::map is a class template. It is not a class in and of itself. It needs template parameters, like you used when you defined the LdapClientMap type. Later, you say std::map::end, and the compiler says that needs parameters, too.

But you probably meant _ldapClientMap.end(). Each map has its own end; end is not a static function, so you need to call it on an instance. If it were static, you would have needed to provide template parameters, just like when you defined the type: std::map<int, LdapClient*>::end.

Rob Kennedy
+1  A: 

std::map::end() is a member function of the container instance and not a universal value, so you'll need to check the result of std::map::find() against _ldapClientMap.end().

Another couple of suggestions to improve the code:

  • Standard C++ containers have value semantics (they want to store the actual object and not a pointer to the object). If you really need to store pointers to LdapClients instead of the LdapClient objects themselves, I would strongly recommend wrapping them in an appropriate smart pointer like boost::shared_ptr (not std::auto_ptr, which will not work). This way, the automatic memory management of the std::map will still work and destroy the objects as intended. If you don't want to use a smart pointer or put the actual LdapClient object into the container, you will have to manually manage the objects' lifetime and call delete when appropriate to prevent memory leaks. My preference would be to change the type of the map to std::map unless the LdapClient objects are polymorphic.
  • Unless you are using a very out of date compiler, checking the result of regular new() against 0 or NULL will not yield any new insights as new throws a std::bad_alloc these days when it can't allocated memory for whatever reason.
  • Instead of using _ldapClientMap[x] = y; to insert a new element, I would use _ldapClientMap.insert(LdapClientMap::value_type(x,y)) as the latter will not overwrite an existing value for key x (which the former will do) and will return 'false' in case the key already exists in the map. That is of course if that is your intention.
Timo Geusch
Thanks for the hints, there were some good comments in there.
Matt H
A: 
LdapClientMap _ldapClientMap;

You should avoid using names with a leading underscore. Technically it is undefined behavior, even if the compiler allows it because by using it you conflict with current or future reserved names.

navigator
It's not. Single leading underscore before lowercase letters is only reserved in global scope; class members and local variables are fine, and he's declaring a member variable. Single leading underscore before a capital letter, and double underscore anywhere in identifier, are reserved in all scopes (hence why all STL implementations use `_Foo` for argument and member names - as standard explicitly says that user code redefining such identifiers via macros is U.B.).
Pavel Minaev
I always use a single underscore for member variables to distinguish them.
Matt H