C++ template to cover const and non-const method

Question

I have a problem with duplication of identical code for const and non-const versions. I can illustrate the problem with some code. Here are two sample visitors, one which modifies the visited objects and one which does not.

struct VisitorRead 
{
    template <class T>
    void operator()(T &t) { std::cin >> t; }
};

struct VisitorWrite 
{
    template <class T> 
    void operator()(const T &t) { std::cout << t << "\n"; }
};

Now here is an aggregate object - this has just two data members but my actual code is much more complex:

struct Aggregate
{
    int i;
    double d;

    template <class Visitor>
    void operator()(Visitor &v)
    {
        v(i);
        v(d);
    }
    template <class Visitor>
    void operator()(Visitor &v) const
    {
        v(i);
        v(d);
    }
};

And a function to demonstrate the above:

static void test()
{
    Aggregate a;
    a(VisitorRead());
    const Aggregate b(a);
    b(VisitorWrite());
}

Now, the problem here is the duplication of Aggregate::operator() for const and non-const versions.

Is it somehow possible to avoid duplication of this code?

I have one solution which is this:

template <class Visitor, class Struct>
void visit(Visitor &v, Struct &s) 
{
    v(s.i);
    v(s.i);
}

static void test2()
{
    Aggregate a;
    visit(VisitorRead(), a);
    const Aggregate b(a);
    visit(VisitorWrite(), b);
}

This means neither Aggregate::operator() is needed and there is no duplication. But I am not comfortable with the fact that visit() is generic with no mention of type Aggregate.

Is there a better way?

Jussive

I think you may have a design problem more than an implementation problem. Having a const method or taking a const variable send a very strong message: "this method doesn't make changes!" Yet having a version that does change things simply means that it will get called in its place. This will only lead to confusion, and eventually bugs. – Hybridism

If you could see the real code, you would see that the design makes sense. The real aggregate has dozens of POD members and the visit function works through them in a prescribed way (conditionals, etc.) - the same way whether we are reading or writing the object. – Jussive

Can't you remove the non const version of Aggregate::operator () ? – Slit

@Slit I could, but then I couldn't use const Aggregate as b in test(). – Jussive

@paperjam, I couldn't get that. removing non-const version will not harm .. correct ? – Slit

So what is the problem with the free function visitor not mentioning Aggregate? The STL does quite a bit of that... just naming the argument as ForwardIterator and expect that user code knows what to do. Alternatively, you could use SFINAE to detect whether the argument is actually a Aggregate or not. – Exsiccate

@David - not a big problem but I think it makes the code harder to understand from a low level view. – Jussive

I don't follow the harder to understand from a low level view. The function is the same, the coupling might be a little too weak (i.e. the type is not mentioned, but if you provide that function as part of the same header as Aggregate, then following the interface principle they do belong to the same type...) – Exsiccate

isn't there some sort of standard proposal in review that exposes the this pointer to solve this problem ? – Hakim

Answer 1

I tend to like simple solutions, so I would go for the free-function approach, possibly adding SFINAE to disable the function for types other than Aggregate:

template <typename Visitor, typename T>
typename std::enable_if< std::is_same<Aggregate,
                                   typename std::remove_const<T>::type 
                                  >::value
                       >::type
visit( Visitor & v, T & s ) {  // T can only be Aggregate or Aggregate const
    v(s.i);
    v(s.d);   
}

Where enable_if, is_same and remove_const are actually simple to implement if you don't have a C++0x enabled compiler (or you can borrow them from boost type_traits)

EDIT: While writing the SFINAE approach I realized that there are quite a few problems in providing the plain templated (no SFINAE) solution in the OP, which include the fact that if you need to provide more than one visitable types, the different templates would collide (i.e. they would be as good a match as the others). By providing SFINAE you are actually providing the visit function only for the types that fulfill the condition, transforming the weird SFINAE into an equivalent to:

// pseudocode, [] to mark *optional*
template <typename Visitor>
void visit( Visitor & v, Aggregate [const] & s ) {
   v( s.i );
   v( s.d );
}

Membranous

Thanks for this. I was able to use the C++20 requires keyword to the same effect, in place of std::enable_if. – Connubial

Answer 2

struct Aggregate
{
    int i;
    double d;

    template <class Visitor>
    void operator()(Visitor &v)
    {
        visit(this, v);
    }
    template <class Visitor>
    void operator()(Visitor &v) const
    {
        visit(this, v);
    }
  private:
    template<typename ThisType, typename Visitor>
    static void visit(ThisType *self, Visitor &v) {
        v(self->i);
        v(self->d);
    }
};

OK, so there's still some boilerplate, but no duplication of the code that depends on the actual members of the Aggregate. And unlike the const_cast approach advocated by (e.g.) Scott Meyers to avoid duplication in getters, the compiler will ensure the const-correctness of both public functions.

Nook

Neat, but ThisType could still be anything, although in a private member function is less nasty. Also there's a lot of boilerplate which I guess I could avoid using CRTP if I have many structs like Aggregate. – Jussive

@paperjam: I suppose you could add static asserts that ThisType is either Aggregate* or Aggregate const*. But instead, I'd just document that the first argument to visit must be this, and leave it to the caller not to be an idiot. Or, document that visit is a helper function for operator(), and must not be called from anywhere else, even from other members of Aggregate. CRTP might reduce boilerplate. Not sure, since the parent class can't call private function visit, and we don't necessarily want it public. – Nook

Actually, who cares that ThisType can be anything? Obviously it can only be something that has the same members as Aggregate, since the body of visit is going to use those members. So worst realistic case, its a derived class of Aggregate, and we fail to visit any extra members that the derived class adds. But that's expected when you try to do this visitor stuff with class hierarchies, subclasses have to be sure to hide/override everything. If someone finds a way to use visit generically, on some other class with the same member names, well then good luck to them ;-) – Nook

@SteveJessop By 'generically' you probably mean a base that is unaware of its derivation. As the others mentioned, CRTP may be an alternative. You may not have a choice in the matter, typically if the instances need to be stored in a heterogeneous container of some kind. Any unbroken chain of uses satisfying LSP (en.wikipedia.org/wiki/Liskov_substitution_principle) can complete a circuit to the original derivation. The basic use is D: B<D> for concrete and final D with respect to B (but Liskov can get you much further.) – Jahdal

A happy coincidence - eli.thegreenplace.net/2011/05/17/… goes over this very problem with CRTP and mixins, allowing some abstraction without sacrificing derived behavior. The container can always store the mixins by type traits to regain some type constraints on implementation, e.g. losing iterator but retaining const_iterator when it's stored alongside const_iterables. – Jahdal

Answer 3

Since your ultimate implementations are not always identical, I don't think there's a real solution for your perceived "problem".

Let's think about this. We have to cater for the situations where Aggregate is either const or non-const. Surely we should not relax that (e.g. by providing only a non-const version).

Now, the const-version of the operator can only call visitors which take their argument by const-ref (or by value), while the non-constant version can call any visitor.

You might think that you can replace one of the two implementations by the other. To do so, you would always implement the const version in terms of the non-const one, never the other way around. Hypothetically:

void operator()(Visitor & v) { /* #1, real work */ }

void operator()(Visitor & v) const
{
  const_cast<Aggregate *>(this)->operator()(v);  // #2, delegate
}

But for this to make sense, line #2 requires that the operation is logically non-mutating. This is possible for example in the typical member-access operator, where you provide either a constant or a non-constant reference to some element. But in your situation, you cannot guarantee that the operator()(v) call is non-mutating on *this!

Therefore, your two functions are really rather different, even though they look formally similar. You cannot express one in terms of the other.

Maybe you can see this another way: Your two functions aren't actually the same. In pseudo-code, they are:

void operator()(Visitor & v) {
  v( (Aggregate *)->i );
  v( (Aggregate *)->d );
}

void operator()(Visitor & v) const {
  v( (const Aggregate *)->i );
  v( (const Aggregate *)->d );
}

---

Actually, coming to think of it, perhaps if you're willing to modify the signature a bit, something can be done:

template <bool C = false>
void visit(Visitor & v)
{
  typedef typename std::conditional<C, const Aggregate *, Aggregate *>::type this_p;
  v(const_cast<this_p>(this)->i);
  v(const_cast<this_p>(this)->d);
}

void operator()(Visitor & v) { visit<>(v); }
void operator()(Visitor & v) const { const_cast<Aggregate *>(this)->visit<true>()(v); }

Enchantress

Makes sense, thanks @Kerrek. Perhaps it is possible to improve my suggested solution bring the type Aggregate into the visit function. Something similar to this: codeguru.com/forum/showthread.php?t=487097 ? – Jussive

@paperjam: I added a pretty crazy hack at the end, check it out :-) The big initial obstruction is that there's nothing in the signature directly that lets us distinguish the two (i.e. v is always non-const); so I put in a template parameter to create two separate static code paths. – Enchantress

I like your second solution but using a bool seems a little clunky. – Jussive

@paperjam: Alternatively, you could maybe use std::is_same<decltype(this), const Aggregate *>::value. – Enchantress

I think it makes sense in certain contexts. The distinction is valid, but the same way you would want perfect forwarding for lvalue/rvalue invariance, CV invariance seems just as useful. As an example, you would expect g=CV(f[i])=CV(f)[i] (operator[] has no side effects of its own, CV distributes, and the same element is accessed.) Any surprises not inherent to g would be better reasons not to overload operator[]. – Jahdal

Answer 4

Normally with this type of thing, it's possibly better to use methods that make sense. For example, load() and save(). They say something specific about the operation that is to be carried out via the visitor. Typically both a const and non-const version is provided (for things like accessors anyway), so it only appears to be duplication, but could save you some headache debugging later down the line. If you really wanted a workaround (which I wouldn't advice), is to declare the method const, and all the members mutable.

Isis

Answer 5

Add visitor trait to tell whether it's modifying or not (const or non-const use). This is used by STL iterators.

Uphroe

Some additional information would probably be beneficial. – Monongahela

Answer 6

You could use const_cast and change VisitorRead's method signature so it also take's const T& as a parameter, but I think that is an ugly solution.

Burress

How would you const_cast<nonconst T *>(& (T) x) if T is already const? ("noncost" keyword used as example). In other words: how to remove the constness of T? – Vannoy

Answer 7

Another solution - require the Visitor class to have a metafunction that adds const when it applies:

template <class Visitor>
static void visit(Visitor &v, typename Visitor::ApplyConst<Aggregate>::Type &a)
{
    v(a.i);
    v(a.d);
}

Jussive