5

What is wrong with this code:

Header:

#include <map>

using namespace std;

template<class T>
class ValueCollection
{
public:
    ValueCollection(void);

    int getValueCount(void);

    map<string, T> Values;
};

Implementation:

#include "ValueCollection.h"

ValueCollection<class T>::ValueCollection(void)
{
}

int ValueCollection<class T>::getValueCount(void)
{
    return Values.size();
}

Test:

#include "ValueCollection.h"

TEST(ValueCollection_TestCases, Default_Constructor_MapIsEmpty)
{
    ValueCollection<int>* target = new ValueCollection<int>;

    int expected = 0;
    int actual = target->getValueCount();

    ASSERT_EQ(expected, actual);
}

This is the error:

Error   1   error C2079: 'std::_Pair_base<_Ty1,_Ty2>::second' uses undefined class 'T'  c:\program files (x86)\microsoft visual studio 10.0\vc\include\utility  167 1   Refactor01
Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
IUnknown
  • 2,596
  • 4
  • 35
  • 52
  • Have you included `` at any point? – Oliver Charlesworth Nov 25 '11 at 16:56
  • Many things wrong. Short answer: You can't separate the definition from the declaration for templates. – Kerrek SB Nov 25 '11 at 16:56
  • 5
    And please please please **never say `using namespace std;`** in a header file. – Kerrek SB Nov 25 '11 at 16:57
  • add `typename` as in ` map Values;`? – zerm Nov 25 '11 at 16:57
  • @KerrekSB: I would simply say, never use `using namespace` at all, and import each symbol piecemeal... *if it really makes sense*. As far as I am concerned, it should be forbidden (we have typedefs) except at function/class scope. – Matthieu M. Nov 25 '11 at 18:47
  • @MatthieuM.: I agree, to an extent: Before C++11, `using` should *only* be used for explicit ADL control and for base class unhiding (this includes some template situations). (With C++11, `using` gains legitimacy as a replacement for `typedef`.) – Kerrek SB Nov 25 '11 at 18:49
  • 1
    @KerrekSB: I am not bashing `using` itself, but the idea of injecting a name directly when aliasing does as good a job (at least at namespace level). In C++11, this is aliasing. – Matthieu M. Nov 25 '11 at 18:56
  • @MatthieuM.: Yes, "injection" vs "aliasing" is nice way to think about it :-) – Kerrek SB Nov 25 '11 at 19:01
  • This thanks goes out to all those who help - I love StackOverflow community. I not only have it working now but understand why – IUnknown Nov 25 '11 at 19:14

3 Answers3

3

implementation should read:

#include "ValueCollection.h"

template <class T>
ValueCollection<T>::ValueCollection(void)
{
}

template <class T>
int ValueCollection<T>::getValueCount(void)
{
    return Values.size();
}
sehe
  • 374,641
  • 47
  • 450
  • 633
  • That'll only *begin* to open Pandora's can of worms, though. – Kerrek SB Nov 25 '11 at 17:00
  • @KerrekSB: we'll see. I think you're overreacting. Nobody says he's putting the 'implementation' in a separate TU. In fact, the OP doesn't say _where_ it goes :). The guy is working Test-Driven too. That is a _pre_ ! – sehe Nov 25 '11 at 17:06
  • Well, she does say "header" and "implementation", so my bet is on "why do I get this linker error" in a minute. I appreciate your optimism, though! – Kerrek SB Nov 25 '11 at 17:08
  • This thanks goes out to all those who help - I love StackOverflow community. I not only have it working now but understand why. – IUnknown Nov 25 '11 at 19:14
3

A couple of problems.

Your most immediate compiler error is caused by incorrect syntax in the implementation of the class template member functions. You must preface definition of a class template member with the template keyword. To wit:

template<class T> ValueCollection<T>::ValueCollection(void)
{
}

template<class T> int ValueCollection<T>::getValueCount(void)
{
    return Values.size();
}

There's another problem too, which will only become apparent later as your program grows. You can't define template functions or classes in one Translation Unit and use them in antoher. The compiler must have the complete definition available in each translation unit.

Typically, the way this is accomplished is by defining template functions directly in the header file, right where they are declared. In your case:

template<class T>
class ValueCollection
{
public:
    ValueCollection(void)
    {
    }

    int getValueCount(void)
    {
        return Values.size();
    }

    map<string, T> Values;
};

That's just one way to accomplish this -- there are others. Another way is to use the so-called "inclusion method":

ValueCollection.h

template<class T>
class ValueCollection
{
public:
    ValueCollection(void);

    int getValueCount(void);

    map<string, T> Values;
};

#include "ValueCollection.hpp:

ValueCollection.hpp

template<class T> ValueCollection<T>::ValueCollection(void)
{
}

template<class T> int ValueCollection<class T>::getValueCount(void)
{
    return Values.size();
}

Yet another method would be to supply a new definition for each translation unit that wants to use the templates, but this would be highly unusual. (In fact, I have never done this in my 15 years of C++ programming)

Community
  • 1
  • 1
John Dibling
  • 99,718
  • 31
  • 186
  • 324
  • This thanks goes out to all those who help - I love StackOverflow community. I not only have it working now but understand why. – IUnknown Nov 25 '11 at 19:14
0

You require the template <class T> before every method in your implementation file too.

Jeff Burdges
  • 4,204
  • 23
  • 46