is it ok to use const_cast in member routines to avoid duplicated code

Question

Is it okay to use const_cast in the following case, or are there any caveats:

class A {
public:
  A() : m_someData(5) {}

  int& get() { return m_someData; }

  const int& get() const { return *const_cast<A*>(this)->get(); }
private:
  int m_someData;
};

The intention is that the get routine may be much more complicated and code duplication should be avoided.

Dexamethasone

Answer 1

No. I wouldn't suggest that. I would suggest you to use const_cast in reverse direction:

int& get() { return const_cast<int&>(const_cast<A const &>(*this).get()); };

const int& get() const {  return m_someData; };

That is, you should call the const member function from non-const overload, not the other way round. That way, you ensure that even the non-const version doesn't change the state of the object and the code in both functions are indeed same (because you call the const version eventually) — and there would be certainly code duplication which you want to avoid.

If you do this the other way round, the compiler will not tell you if you modify the object in non-const version of the function and it would be incorrect to call such function from const function.

Drayman

I don't think it matters which one calls the other one; either way, you have to be certain that neither of them does any modifications. When the const delegates to the non-const, you must ensure the non-const does no modifications. When the non-const delegates to the const, you must ensure the const does not return anything actually const. And with the const overload calling the non-const one, you at least save one const_cast. – Indeterminism

@Angew: non-const version might modify the object. if you do so, the compiler will not tell you. so it is not safe to call the non-const function from const function. – Drayman

@Angew i think he is right. if i cast the this to nonconst and call the non const get() then the compiler does not enforce that the this object is not changed in the get – Dexamethasone

If you do the work in the const overload, the compiler will hopefully stop most modifications because *this is const. If you do the work in the non-const overload you may accidentally invoke undefined behaviour. – Zurek

I think decision really concerns the programmer than the compiler. If the non-const version is as proposed in this answer, it's just one line and I can easily check the object is not modified. If the non-const version is the one that actually does the job and is say 10 lines long, it's easy to miss a point where state is modified. So I'd rather have the compiler do the checking for me. – Shaky

Answer 2

As user @Nawaz has pointed out in their answer, it is better to const_cast the other way, which has less potential for making a mistake. This can be made slightly easier with std::as_const:

int& get() { return const_cast<int&>(std::as_const(*this).get()); }

However, many style guides ban const_cast (e.g. CppCoreGuidelines) for such purposes because it is difficult to tell whether what you're doing is correct at first glance. Consider the following alternatives.

Static member function template (C++98 - C++20)

private:
  // C++20 abbreviated function template.
  // In C++17 and prior, use template <typename Self>
  // Implementable in C++98 (see comments), but much more verbose.
  static auto& get(auto &self) { return self.m_someData; }

public:
  int& get() { return get(*this); }

  const int& get() const { return get(*this); }

This solution works because *this will be either A or const A depending on non-static member function is calling the static member function template. Many developers prefer this, because there is no const_cast at all, and so it is trivial to verify that this code is const-correct. However, get being a template may increase code size.

Deducing this (C++23)

With this C++23 feature, we can accept this as a function parameter and handle all combinations of cv-qualifiers and reference qualifiers in one function. This is similar to the first solution, however, we only have to write a single function.

// returns int&, const int&, volatile int&, or const volatile int&
auto& get(this auto &&self) { return self.m_someData; }

You could extend this further, so that when you have an A rvalue, you return int&&. For an int, this would be useless, but it may make sense for other types of members:

// returns int&, const int&, volatile int&, const volatile int&,
//   int&&, const int&&, volatile int&&, or const volatile int&&
template <typename Self>
auto&& get(this Self &&self) { return std::forward<Self>(self).m_someData; }

Kassandra

have not seen auto in C++98 ;-) - i currently not see, how to replace the return type auto in pre-auto-compilers (to still get the auto-const-ness) But nice - I just read about "Deducing this" a few days ago. – Dexamethasone

@Dexamethasone you would have to implement a type trait equivalent to std::add_const in C++98. The return type would look something like: typename add_const<typename Self::some_data_type>::type. – Kassandra

Actually it would be a bit more complicated: typename conditional<is_const<Self>::value, typename add_const<typename Self::some_data_type>::type, some_data_type>::type. It would be simpler to use a copy_cv_qual trait which takes the cv qualifiers from one type and applies it to another: typename copy_cv_qual<some_data_type, Self>::type. – Kassandra

I hope, I never have to deal with pre c++17 compiler again 🙈 – Dexamethasone