Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sometimes I run across the problem of having two member function overloads that only differ in the constness of this (and the return type, but that is less important):

int& operator[](int);
const int& operator[](int)const;

Most of the time these are one-liners, but even then, I hate the code duplication. That's why I like to use (if possible) the const version to implement the non const version via const_cast:

const int& MyClass::operator[](int index)const {
    return const_cast<int&>(const_cast<MyClass&>(*this)[index]);
}

The first const_cast is required (at least I think so), but I wonder if the second one can be circumvented and if so, if the code is more readable then.

How do you solve this problem?

share|improve this question
    
There are several good answers on StackOverflow: see How do I remove code duplication between similar const and non-const member functions? –  ChrisW Mar 15 at 19:30

3 Answers 3

up vote 10 down vote accepted

First you have it the wrong way around.

You should not use the mutating version to supply data to the non-mutating function:

const int& MyClass::operator[](int index)const {
    return const_cast<int&>(const_cast<MyClass&>(*this)[index]);
}

What happens if operator[] is modified and does mutate slightly the internal state? By casting away the constness you have opened up your class to undefined behavior. You should NEVER cast away constness. If you must you can use const_cast to add constness, but removing it is so dangerous that this should be looked at as breaking code.

If you had written it the other way around:

int& MyClass::operator[](int index) {
    return const_cast<int&>(const_cast<MyClass const&>(*this)[index]);
}

This is better. Because if you change the operator[] so that it actually mutates the class it will generate a compiler error and thus prevent you from accidentally getting undefined behavior.

But personally I prefer to add another helper method and thus remove all athe casting:

private:
// NOTE: This is not a perfect solution for all situations.
//       But it works for 99% of situations and keeps the code clean
//       For the remaining situations switch back to use const cast
//       **BUT** always add never remove constness from an object.
// Never make public as it is const and gives away an internal reference.
// Only use via the public API
int& get(int index) const
{
    return stuff[index];
}

public:
// Keep the API simple/clean and DRY
// move all the work to private methods.
int&       operator[](int index)       { return get(index);}
int const& operator[](int index) const { return get(index);}

Answer to comment below:

I would not use it for const/non-const iterators

const_iterator begin() const {return const_iterator(<X>);}
iterator       begin()       {return iterator(<X>);}
share|improve this answer
    
Would you show how that works if get is supposed to return a const or non-const iterator type, instead of a const or non-reference? Perhaps get return a non-const iterator instance, and you define the const iterator type so that it can be explicitly or implicitly created from a non-const iterator instance? –  ChrisW Mar 15 at 22:46
    
@ChrisW: Hope the edit helps. –  Loki Astari Mar 16 at 1:02
    
It seems you have misplaced the ( in your comment answer. @ChrisW: As the code in question is usually about own code, you can write your own const_cast that converts the iterators. If you are dealing with (automatically) const standard contains in C++11 you can use the erase function to convert const iterators to mutable iterators in O(1). –  Nobody Mar 17 at 9:39

As ChrisW commented, this has been covered in depth at Stack Overflow. You're very close, but you should actually have the non-const version cast away the const-ness of the const version rather than how you're doing it.

If you're calling a non-const method, you are operating on a non-const object. This means that the const cast is technically valid since you are assured that the underlying object is non-const anyway.


Note that there are other options that are potentially cleaner from a "I should always avoid const_cast!" point of view. Personally, I think this is one of the legitimate uses of const_cast, and it is the approach I use. If you'd like to see more information on the other options, see the Stack Overflow post.

share|improve this answer
    
Hm I just reproduced the code from the top of my head and did not notice I had done it the other way around. Of course you are right. The non const code could do manipulations that we don't want the const code to suffer from. –  Nobody Mar 15 at 20:55

One (C++11) solution is to "hide" the ugly second const_cast with a macro (shudder):

#include <type_traits> 
#define CONST_THIS const_cast< \
                      std::add_pointer< \
                          std::add_const< \
                              std::remove_pointer< \
                                  decltype(this)>::type>::type>::type>(this)

And use it like this:

int& operator[](int index) {
    return const_cast<int&>((*CONST_THIS)[index]);
}

The code in the macro uses type traits to add constness to the this pointer. To do so it must at first strip the pointer (to apply the const to the pointed to type) and then make it a pointer again to use the macro as if it where the this pointer in a const member function.

share|improve this answer
3  
Does any of the downvoters mind to give an explanation? I dislike macros but I find the code more readable that way and it certainly improves over the "useless" repetition of the type of this to use the const_cast. –  Nobody Mar 17 at 9:42

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.