Cannot insert user-defined into an encapsulated vector using pimpl idiom

Question

I have an issue on using my Push function on the TokenList class where I encapsulated the vector of Tokens. Whenever I call the Push function, the member variable word inside Token class was empty even when I initialize the Token variable properly. I've also tried using integer and it stores garbage number.

Token class

class Token {
private:
    class Impl;
    std::unique_ptr<Impl> impl;
public:
    Token();
    Token(const Token& token);
    Token(const std::string& word);
    ~Token();
    std::string GetWord();
    void SetWord(const std::string& word);
};

TokenList class

class TokenList {
private:
    class Impl;
    std::unique_ptr<Impl> impl;
public:
    TokenList();
    TokenList(const TokenList& tokenList);
    ~TokenList();
    std::size_t Size() const;
    void Push(const Token& token);
    Token& operator[](int i);
    const Token& operator[](int i) const;
};

Implementation

class TokenList::Impl {
private:
    std::vector<Token> tokens;
public:
/* .... */
    void push(const Token& token) {
        tokens.push_back(token);
    }
};
void TokenList::Push(const Token& token) { impl->push(token); }

Main

TokenList tokens;
Token s1 ("hello");
Token s2 ("world");
tokens.Push(s1);
tokens.Push(s2);
for (int i = 0; i < tokens.Size(); ++i)
   cout << i << ": " << tokens[i].GetWord() << endl;

Here is the output when I run the main:

0:
1:

I've tried using string instead of TokenList as the type parameter for the vector and it works perfectly. When I trace the issue, it was in the Push function not getting the correct passed Token value. So what causes the TokenList's Push functioon to not take the member values of the Token class?

Must be a problem in your copy operations. Show the copy ctor, and of course follow the [Rule of Three](http://stackoverflow.com/q/4172722/1782465) and implement a copy assignment op as well. — Angew is no longer proud of SO, Feb 13 '14 at 10:11
You're not correctly copying the `Token`. You have pointers in the class, so you must implement the copy constructor and assignment operator to copy the state. — Marius Bancila, Feb 13 '14 at 10:16

score 0 · Accepted Answer · answered Feb 13 '14 at 10:44

I have implemented all the missing code and I cannot reproduce your problem. Note that I added a copy assignment operator as this is required for types that are stored in a vector that are not movable. I have published the code below so that you can compare to your implementation. (This uses some C++11 features)

#include <string>
#include <vector>
#include <iostream>
#include <memory>


class Token {
private:
    struct Impl;
    std::unique_ptr<Impl> impl;
public:
    Token();
    Token(const Token& token);
    Token(const std::string& word);
    Token& operator=(const Token& token);
    std::string GetWord();
    void SetWord(const std::string& word);
};

class TokenList {
private:
    struct Impl;
    std::unique_ptr<Impl> impl;
public:
    TokenList();
    TokenList(const TokenList& tokenList);
    std::size_t Size() const;
    void Push(const Token& token);
    Token& operator[](int i);
    const Token& operator[](int i) const;
};

// Token Implementation
struct Token::Impl
{
    std::string Word;
};

Token::Token() : impl{ std::make_unique<Impl>() }
{
}

Token::Token(const Token& token) : Token{}
{
    impl->Word = token.impl->Word;
}

Token::Token(const std::string& word) : Token{}
{
    impl->Word = word;
}

Token& Token::operator=(const Token& token)
{
    impl->Word = token.impl->Word;
    return *this;
}

std::string Token::GetWord()
{
    return impl->Word;
}

void Token::SetWord(const std::string& word)
{
    impl->Word = word;
}

// TokenList Implementation
struct TokenList::Impl 
{
    std::vector<Token> tokens;

    void push(const Token& token) 
    {
        tokens.push_back(token);
    }
};

TokenList::TokenList() : impl{ std::make_unique<Impl>() }
{
}

TokenList::TokenList(const TokenList& tokenList) : TokenList{}
{
    impl->tokens = tokenList.impl->tokens;
}

void TokenList::Push(const Token& token)
{ 
    impl->push(token); 
}

std::size_t TokenList::Size() const
{
    return impl->tokens.size();
}

Token& TokenList::operator[](int i)
{
    return impl->tokens[i];
}

const Token& TokenList::operator[](int i) const
{
    return impl->tokens[i];
}

int main()
{
    TokenList tokens;
    Token s1("hello");
    Token s2("world");
    tokens.Push(s1);
    tokens.Push(s2);
    for (int i = 0; i < tokens.Size(); ++i)
        std::cout << i << ": " << tokens[i].GetWord() << std::endl;

}

Is it the best way to make the variables public in the pimpl class? This way it works perfect. — Kenneth Bastian, Feb 13 '14 at 11:07
I tend to make everything in the implementation class public if it is a trivial class. It is an implementation detail after all. But it can affect testability and maintainability. As always there is no one correct answer for all situations. — Peter R, Feb 13 '14 at 11:11

Cannot insert user-defined into an encapsulated vector using pimpl idiom

1 Answers1