How do I remove code duplication between similar const and non-const member functions?
Asked Answered
P

21

320

Let's say I have the following class X where I want to return access to an internal member:

class Z
{
    // details
};

class X
{
    std::vector<Z> vecZ;

public:
    Z& Z(size_t index)
    {
        // massive amounts of code for validating index

        Z& ret = vecZ[index];

        // even more code for determining that the Z instance
        // at index is *exactly* the right sort of Z (a process
        // which involves calculating leap years in which
        // religious holidays fall on Tuesdays for
        // the next thousand years or so)

        return ret;
    }
    const Z& Z(size_t index) const
    {
        // identical to non-const X::Z(), except printed in
        // a lighter shade of gray since
        // we're running low on toner by this point
    }
};

The two member functions X::Z() and X::Z() const have identical code inside the braces. This is duplicate code and can cause maintenance problems for long functions with complex logic.

Is there a way to avoid this code duplication?

Principal answered 23/9, 2008 at 20:47 Comment(2)
In this example I would return a value in the const case so you can't the refactoring below. int Z() const { return z; }Chinquapin
For fundamental types, you're absolutely correct! My first example wasn't very good. Let's say that instead we're returning some class instance instead. (I updated the question to reflect this.)Principal
P
75

Yes, it is possible to avoid the code duplication. You need to use the const member function to have the logic and have the non-const member function call the const member function and re-cast the return value to a non-const reference (or pointer if the functions returns a pointer):

class X
{
   std::vector<Z> vecZ;

public:
   const Z& z(size_t index) const
   {
      // same really-really-really long access 
      // and checking code as in OP
      // ...
      return vecZ[index];
   }

   Z& z(size_t index)
   {
      // One line. One ugly, ugly line - but just one line!
      return const_cast<Z&>( static_cast<const X&>(*this).z(index) );
   }

 #if 0 // A slightly less-ugly version
   Z& Z(size_t index)
   {
      // Two lines -- one cast. This is slightly less ugly but takes an extra line.
      const X& constMe = *this;
      return const_cast<Z&>( constMe.z(index) );
   }
 #endif
};

NOTE: It is important that you do NOT put the logic in the non-const function and have the const-function call the non-const function -- it may result in undefined behavior. The reason is that a constant class instance gets cast as a non-constant instance. The non-const member function may accidentally modify the class, which the C++ standard states will result in undefined behavior.

Principal answered 23/9, 2008 at 20:47 Comment(11)
Wow... that's horrible. You just increased the amount of code, decreased the clarity, and added two stinkin' const_cast<>s. Perhaps you have an example in mind where this actually makes sense?Norseman
Yes, that hideous beast is much worse than the code duplication.Expect
Ok, you modified it to work on something other than a simple integer. The "duplicating" code is still more clear than the non-duplicating code.Norseman
Hey don't ding this!, it may be ugly, but according to Scott Meyers, it is (almost) the correct way. See Effective C++, 3d ed, Item 3 under the heading "Avoiding duplication in const and non-cost member functions.Kozak
While I understand that the solution may be ugly, imagine that the code that determines what to return is 50 lines long. Then duplication is highly undesirable -- especially when you have to re-factor the code. I've encountered this many times in my career.Principal
@Kozak -- Out of curiosity, what is incorrect? And how do Scott Meyers and I differ?Principal
Kevin - you need at least an edit #6 - in the ugly set of casts you have "const_cast<int&>" which should be "const_cast<Z&>"Terrapin
The difference between this and Meyers is that Meyers has static_cast<const X&>(*this). const_cast is for removing const, not adding it.Roots
As per C++ standard, it is invalid to use const_cast to remove the const qualifier from an object that was originally created as const. For all I know, this code invokes undefined behavior.Information
@VioletGiraffe we know that the object wasn't originally created const, as it is a non-const member of a non const object, which we know because we are in a non-const method of said object. The compiler doesn't make this inference, it follows a conservative rule. Why do you think const_cast exists, if not for this kind of situation?Abysm
@SteveJessop Nope. const_cast is for both adding and removing const. Read the proposal for as_const (open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0007r0.html). It explicitly mentions const_cast< const T & >( object ) as a workable but hard-to-use alternative.Paillette
K
253

For a detailed explanation, please see the heading "Avoid Duplication in const and Non-const Member Function," on p. 23, in Item 3 "Use const whenever possible," in Effective C++, 3d ed by Scott Meyers, ISBN-13: 9780321334879.

alt text

Here's Meyers' solution (simplified):

struct C {
  const char & get() const {
    return c;
  }
  char & get() {
    return const_cast<char &>(static_cast<const C &>(*this).get());
  }
  char c;
};

The two casts and function call may be ugly, but it's correct in a non-const method as that implies the object was not const to begin with. (Meyers has a thorough discussion of this.)

Kozak answered 23/9, 2008 at 21:24 Comment(15)
Nobody ever got fired for following Scott Meyers :-)Roots
const_cast should almost never be used. It breaks const-ness. This is especialy true on embeeded systems where you have ROM. In general it's a bad idea to use const_cast.Kynthia
Ouch... you may already be in trouble if you're counting on const == ROM. Compilers just aren't good enough to prove such a strong assertion.Nuclear
witkamp is correct that in general it's bad to use const_cast. This is a specific case where it isn't, as Meyers explains. @Adam: ROM => const is fine. const == ROM is obviously nonsense since anyone can cast non-const to const willy-nilly: it's equivalent to just choosing not to modify something.Roots
In general I would suggest using const_cast instead of static_cast to add const since it prevents you from changing the type accidentally.Hydroxyl
@witkamp You are missing the point, there are two kinds of constness: physical and logical.Drusy
What if your const char & get() const returns something that is actual declared const? Then your non-const version of get will effectively const_cast something that was never supposed to be accessible for writing.Feldstein
@HelloGoodbye: I think Meyers assumes a modicum of intelligence from the designer of the class interface. If get()const returns something that was defined as a const object, then there should not be a non-const version of get() at all. Actually my thinking on this has changed over time: the template solution is the only way to avoid duplication and get compiler-checked const-correctness, so personally I would no longer use a const_cast in order to avoid duplicating code, I'd choose between putting the duped code into a function template or else leaving it duped.Roots
The following two templates help enormously with the readability of this solution: template<typename T> const T& constant(T& _) { return const_cast<const T&>(_); } and template<typename T> T& variable(const T& _) { return const_cast<T&>(_); }. Then you can do: return variable(constant(*this).get());Remains
Sure, absolutely equivalent, but I consider this variant slightly more aesthetic: return const_cast<char &>(const_cast<const C *>(this)->get());Norvall
@CaseyRodarmor Now with C++17 std::as_const() is better.Forficate
@GregRogers re: "In general I would suggest using const_cast instead of static_cast to add const since it prevents you from changing the type accidentally." That's wrong, since const_cast is for casting away the const. For casting to a const, static_cast should be used. This is taken verbatim from Scott Meyers' book.Senhorita
Note that this will only work on two member functions that return exactly the same type (i.e. will not work if one returns char & and the other returns char). Went down an awful rabbit hole trying to figure out why this wasn't working.Rowlandson
Instead making the non-const version calls the const one, is it save to do it in a reversed way - let the const version call the non-const version. I can perform a const_cast to this first, removing its const qualifier, then let it call the non-const version, finally cast the returned reference back to const.Polestar
@Polestar that is NOT safe and it's UB. The only safe way is to make the non-const call the const-version and remove the const with const_cast.Tabitha
T
104

C++17 has updated the best answer for this question:

T const & f() const {
    return something_complicated();
}
T & f() {
    return const_cast<T &>(std::as_const(*this).f());
}

This has the advantages that it:

  • Is obvious what is going on
  • Has minimal code overhead -- it fits in a single line
  • Is hard to get wrong (can only cast away volatile by accident, but volatile is a rare qualifier)

If you want to go the full deduction route then that can be accomplished by having a helper function

template<typename T>
constexpr T & as_mutable(T const & value) noexcept {
    return const_cast<T &>(value);
}
template<typename T>
constexpr T * as_mutable(T const * value) noexcept {
    return const_cast<T *>(value);
}
template<typename T>
constexpr T * as_mutable(T * value) noexcept {
    return value;
}
template<typename T>
void as_mutable(T const &&) = delete;

Now you can't even mess up volatile, and the usage looks like

decltype(auto) f() const {
    return something_complicated();
}
decltype(auto) f() {
    return as_mutable(std::as_const(*this).f());
}
Tenantry answered 18/11, 2017 at 17:51 Comment(10)
Note that "as_mutable" with the const rvalue overload deleted (which is generally preferable) prevents the last example from working if f() returns T instead of T&.Alphabetize
@MaxTruxa: Yes, and this is a good thing. If it just compiled, we would have a dangling reference. In the case where f() returns T, we don't want to have two overloads, the const version alone is sufficient.Tenantry
Very true, I apologize for my full-on brain fart yesterday, no idea what I was thinking about when I wrote that comment. I was looking at a const/mutable getter pair returning a shared_ptr. So what I actually needed was something like as_mutable_ptr which looks almost identical to as_mutable above, except that it takes and returns a shared_ptr and uses std::const_pointer_cast instead of const_cast.Alphabetize
If a method returns T const* then this would bind to T const* const&& rather than binding to T const* const& (at least in my testing it did). I had to add an overload for T const* as the argument type for methods returning a pointer.Festa
On further reflection about the reasoning for removing the T&& overload in the first place, I opted to just use a separate method. While it's nice to think that the compiler would catch any mistakes of returning a dangling reference, I think it's best to just be slightly more verbose where needed.Festa
@monkey0506: I have updated my answer to support pointers as well as referencesTenantry
You mention that your first solution (using std::as_const) might cast away volatile by accident. Does the above (Scott Meyer) answer also suffer from that issue?Nucleotidase
Any solution that uses const_cast has that risk, theoretically. Putting the const_cast into a single function that deduces the exact type and uses that deduced type as the argument to const_cast makes it impossible (unless there is a bug in my implementation of as_mutable). Given the rarity of volatile, it's probably not a serious problem, though.Tenantry
It would be ok to add this for std::shared_ptr? template<typename T> constexpr std::shared_ptr<T> as_mutable( std:shared_ptr<const T> value) noexcept { return std::const_pointer_cast<T>(value); }Octodecillion
@Octodecillion I'd be hesitant to do anything more than references and pointers (and even supporting pointers is kind of suspect). If you want to do something fancier, it's a different function. Consider the generic use of this function: I have a T const & x that I know references mutable data, so I do as_mutable(x). I would expect the value returned by that function is assignable. If I add in your overload and T happens to be a std::shared_ptr, then my function behaves differently.Tenantry
P
75

Yes, it is possible to avoid the code duplication. You need to use the const member function to have the logic and have the non-const member function call the const member function and re-cast the return value to a non-const reference (or pointer if the functions returns a pointer):

class X
{
   std::vector<Z> vecZ;

public:
   const Z& z(size_t index) const
   {
      // same really-really-really long access 
      // and checking code as in OP
      // ...
      return vecZ[index];
   }

   Z& z(size_t index)
   {
      // One line. One ugly, ugly line - but just one line!
      return const_cast<Z&>( static_cast<const X&>(*this).z(index) );
   }

 #if 0 // A slightly less-ugly version
   Z& Z(size_t index)
   {
      // Two lines -- one cast. This is slightly less ugly but takes an extra line.
      const X& constMe = *this;
      return const_cast<Z&>( constMe.z(index) );
   }
 #endif
};

NOTE: It is important that you do NOT put the logic in the non-const function and have the const-function call the non-const function -- it may result in undefined behavior. The reason is that a constant class instance gets cast as a non-constant instance. The non-const member function may accidentally modify the class, which the C++ standard states will result in undefined behavior.

Principal answered 23/9, 2008 at 20:47 Comment(11)
Wow... that's horrible. You just increased the amount of code, decreased the clarity, and added two stinkin' const_cast<>s. Perhaps you have an example in mind where this actually makes sense?Norseman
Yes, that hideous beast is much worse than the code duplication.Expect
Ok, you modified it to work on something other than a simple integer. The "duplicating" code is still more clear than the non-duplicating code.Norseman
Hey don't ding this!, it may be ugly, but according to Scott Meyers, it is (almost) the correct way. See Effective C++, 3d ed, Item 3 under the heading "Avoiding duplication in const and non-cost member functions.Kozak
While I understand that the solution may be ugly, imagine that the code that determines what to return is 50 lines long. Then duplication is highly undesirable -- especially when you have to re-factor the code. I've encountered this many times in my career.Principal
@Kozak -- Out of curiosity, what is incorrect? And how do Scott Meyers and I differ?Principal
Kevin - you need at least an edit #6 - in the ugly set of casts you have "const_cast<int&>" which should be "const_cast<Z&>"Terrapin
The difference between this and Meyers is that Meyers has static_cast<const X&>(*this). const_cast is for removing const, not adding it.Roots
As per C++ standard, it is invalid to use const_cast to remove the const qualifier from an object that was originally created as const. For all I know, this code invokes undefined behavior.Information
@VioletGiraffe we know that the object wasn't originally created const, as it is a non-const member of a non const object, which we know because we are in a non-const method of said object. The compiler doesn't make this inference, it follows a conservative rule. Why do you think const_cast exists, if not for this kind of situation?Abysm
@SteveJessop Nope. const_cast is for both adding and removing const. Read the proposal for as_const (open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0007r0.html). It explicitly mentions const_cast< const T & >( object ) as a workable but hard-to-use alternative.Paillette
T
40

C++23 has updated the best answer for this question thanks to explicit object parameters.

struct s {
    auto && f(this auto && self) {
        // all the common code goes here
    }
};

A single function template is callable as a normal member function and deduces the correct reference type for you. No casting to get wrong, no writing multiple functions for something that is conceptually one thing.


Note: this feature was added by P0847: Deducing this.

Tenantry answered 7/10, 2021 at 18:44 Comment(3)
It's worth noting that this member function also accepts volatile-qualified objects, and it accepts any value category of expression. This makes it more powerful than just a const/non-const pair of functions.Sleazy
Also note that weird behaviour can happen with inheritance since self is not always the class, but could be something derived from it, and you run into problems with name lookup. So you have to write things like return self.X::vecZ[index];, and your member function won't work for private/protected inheritance, e.g.: godbolt.org/z/sfWqKq4K6 .Knowitall
@Knowitall As mentioned in the paper, the private inheritance problem is supposed to be solved with the use of a C-style cast. Interestingly, the only type of cast that can break through access protection and perform a formally correct upcast.Paternalism
B
37

I think Scott Meyers' solution can be improved in C++11 by using a tempate helper function. This makes the intent much more obvious and can be reused for many other getters.

template <typename T>
struct NonConst {typedef T type;};
template <typename T>
struct NonConst<T const> {typedef T type;}; //by value
template <typename T>
struct NonConst<T const&> {typedef T& type;}; //by reference
template <typename T>
struct NonConst<T const*> {typedef T* type;}; //by pointer
template <typename T>
struct NonConst<T const&&> {typedef T&& type;}; //by rvalue-reference

template<typename TConstReturn, class TObj, typename... TArgs>
typename NonConst<TConstReturn>::type likeConstVersion(
   TObj const* obj,
   TConstReturn (TObj::* memFun)(TArgs...) const,
   TArgs&&... args) {
      return const_cast<typename NonConst<TConstReturn>::type>(
         (obj->*memFun)(std::forward<TArgs>(args)...));
}

This helper function can be used the following way.

struct T {
   int arr[100];

   int const& getElement(size_t i) const{
      return arr[i];
   }

   int& getElement(size_t i) {
      return likeConstVersion(this, &T::getElement, i);
   }
};

The first argument is always the this-pointer. The second is the pointer to the member function to call. After that an arbitrary amount of additional arguments can be passed so that they can be forwarded to the function. This needs C++11 because of the variadic templates.

Bengal answered 27/5, 2013 at 20:43 Comment(9)
It's a shame we don't have std::remove_bottom_const to go with std::remove_const.Bumptious
I don't like this solution because it still embeds a const_cast. You could make getElement a template itself, and use the trait of the type inside to mpl::conditional types you need, like iterators or constiterators if needed. The real problem is how to generate a const version of a method when this part of the signature cannot be templatized ?Engelhardt
@v.oddou: std::remove_const<int const&> is int const & (remove top-level const qualification), hence the gymnastics of NonConst<T> in this answer. Putative std::remove_bottom_const could remove the bottom-level const qualification, and do precisely what NonConst<T> does here: std::remove_bottom_const<int const&>::type => int&.Bumptious
@v.oddou: I think you can't get around two separate declarations for the exact reason you've mentioned.Bengal
And nobody should use this solution in production software. It is too "tricky". Either duplicate your code if it is simple enough, or use Scott Meyer's solution, because it is an ugly but well-known idiom.Bengal
This solution does not work well, if getElement is overloaded. Then the function pointer cannot be resolved without giving the template parameters explicitly. Why?Rode
I agree with John. To support, here is a test : ideone.com/fwqmsB. It currently compiles, but if uncomment /**/ (the overload function), it will be no longer compilable. It would be nice if this solution is improved a little further.Makebelieve
@Rode This is a problem with C++ overload resolution for member function pointers (see also #2942926). Currently this can only be solved on the caller side. You don't have to specify all template parameters though. Only the first one TConstReturn works too. Like: return likeConstVersion<int const&>(this, &T::getElement, i);Bengal
You need to fix you answer to use C++11 perfect forwarding: likeConstVersion(TObj const* obj, TConstReturn (TObj::*memFun)(TArgs...) const, TArgs&&... args) { return const_cast<typename NonConst<TConstReturn>::type>((obj->*memFun)(std::forward<TArgs>(args)...)); } Complete: gist.github.com/BlueSolei/bca26a8590265492e2f2760d3cefcf83Oriental
T
33

Nice question and nice answers. I have another solution, that uses no casts:

class X {

private:

    std::vector<Z> v;

    template<typename InstanceType>
    static auto get(InstanceType& instance, std::size_t i) -> decltype(instance.get(i)) {
        // massive amounts of code for validating index
        // the instance variable has to be used to access class members
        return instance.v[i];
    }

public:

    const Z& get(std::size_t i) const {
        return get(*this, i);
    }

    Z& get(std::size_t i) {
        return get(*this, i);
    }

};

However, it has the ugliness of requiring a static member and the need of using the instance variable inside it.

I did not consider all the possible (negative) implications of this solution. Please let me know if any.

Tricolor answered 8/12, 2013 at 11:16 Comment(15)
Well, lets go with the simple fact that you added more boilerplate. If anything, this should be used as an example of why the language needs a way to modify the function qualifiers along with the return type auto get(std::size_t i) -> auto(const), auto(&&). Why '&&'? Ahh, so I can say: auto foo() -> auto(const), auto(&&) = delete;Selfexamination
gd1 : exactly what I had in mind. @Selfexamination and exactly what I concluded too.Engelhardt
@Selfexamination the syntax should be incorporating this keyword. I suggest template< typename T > auto myfunction(T this, t args) -> decltype(ident) The this keyword will be recognized as the implicit object instance argument and let the compiler recognize that myfunction is a member or T. T will be auto deduced on the call site, which will always be the type of the class, but with free cv qualification.Engelhardt
This solution goes in par with https://mcmap.net/q/95311/-perfect-forwarding-to-a-member-function-of-a-data-member. That is, a static member function to interpolate on *this qualifiers using universal references.Monosyllabic
That solution has also the advantage (versus the const_cast one) to allow to return iterator and const_iterator.Precipitous
If implementation is moved in cpp file (and as the method to not duplicated should not be trivial, it would probably be the case), the static can be done at file scope instead of class scope. :-)Precipitous
@Precipitous mmm private members?Tricolor
@gd1: You can still pass them in addition/replacement to *this, if appropriate.Precipitous
@Precipitous a bit uglyTricolor
return get_impl(v, i); vs return get_impl(*this, i);is not IMO, but return get_impl(*this, v, member1, member2, i); would be, and in that case, template static member is still available (and can be implemented into cpp file (with the associated getters) as used only at 2 places).Precipitous
This is the best way I know of, definitely should be the accepted answer. I was typing it myself, but then already found this one. Here's a C++17 example, with optional SFINAE test added: godbolt.org/z/mMK4r3Codie
I like this solution best. It avoids many hidden gotchas. Cleverness can get you 99% safety with const casting but there's a few edge cases lurking in the shadows.Ishmul
This is the solution to use (+/- adjustements) if you have side effects btw. Like when implementing a non-dynamic visitor pattern for instance, where visitor can do different things depending on object's constness (read when const, write when non-const).Avow
An excellent solution, avoids casting which could be typed incorrectly leading to a bug, and avoids macros which pollute the global name space. I don't consider it any more boiler plate than the other answers and is unlikely to cause an error. It would be great if there was another way to set the return type without using decltype on the instance member.Elman
@Elman Thanks. This answer was written in 2013: verbose trailing decltype likely no longer required, decltype(auto) will probably get it done.Tricolor
R
24

A bit more verbose than Meyers, but I might do this:

class X {

    private:

    // This method MUST NOT be called except from boilerplate accessors.
    Z &_getZ(size_t index) const {
        return something;
    }

    // boilerplate accessors
    public:
    Z &getZ(size_t index)             { return _getZ(index); }
    const Z &getZ(size_t index) const { return _getZ(index); }
};

The private method has the undesirable property that it returns a non-const Z& for a const instance, which is why it's private. Private methods may break invariants of the external interface (in this case the desired invariant is "a const object cannot be modified via references obtained through it to objects it has-a").

Note that the comments are part of the pattern - _getZ's interface specifies that it is never valid to call it (aside from the accessors, obviously): there's no conceivable benefit to doing so anyway, because it's 1 more character to type and won't result in smaller or faster code. Calling the method is equivalent to calling one of the accessors with a const_cast, and you wouldn't want to do that either. If you're worried about making errors obvious (and that's a fair goal), then call it const_cast_getZ instead of _getZ.

By the way, I appreciate Meyers's solution. I have no philosophical objection to it. Personally, though, I prefer a tiny bit of controlled repetition, and a private method that must only be called in certain tightly-controlled circumstances, over a method that looks like line noise. Pick your poison and stick with it.

[Edit: Kevin has rightly pointed out that _getZ might want to call a further method (say generateZ) which is const-specialised in the same way getZ is. In this case, _getZ would see a const Z& and have to const_cast it before return. That's still safe, since the boilerplate accessor polices everything, but it's not outstandingly obvious that it's safe. Furthermore, if you do that and then later change generateZ to always return const, then you also need to change getZ to always return const, but the compiler won't tell you that you do.

That latter point about the compiler is also true of Meyers's recommended pattern, but the first point about a non-obvious const_cast isn't. So on balance I think that if _getZ turns out to need a const_cast for its return value, then this pattern loses a lot of its value over Meyers's. Since it also suffers disadvantages compared to Meyers's, I think I would switch to his in that situation. Refactoring from one to the other is easy -- it doesn't affect any other valid code in the class, since only invalid code and the boilerplate calls _getZ.]

Roots answered 23/9, 2008 at 22:2 Comment(11)
This still has the problem that the thing you return may be constant for a constant instance of X. In that case, you still require a const_cast in _getZ(...). If misused by later developers, it can still lead to UB. If the thing that is being returned is 'mutable', then this is a good solution.Principal
When I said that '_getZ(...)' may be mis-used, I meant that if a future developer didn't understand that it was only to be used by the public functions and called it directly, but modified the value in a constant instance of X, then that could lead to UB. This is possible if not documented well.Principal
Any private function (heck, public ones too) can be mis-used by later developers, if they choose to ignore the BLOCK CAPITAL instructions on its valid use, in the header file and also in Doxygen etc. I can't stop that, and I don't consider it my problem since the instructions are easy to understand.Roots
-1: This doesn't work in many situations. What if something in the _getZ() function is an instance variable? The compiler (or at least some compilers) will complain that since _getZ() is const, any instance variable referenced within is const too. So something would then be const (it would be of type const Z&) and could not be converted to Z&. In my (admittedly somewhat limited) experience, most of the time something is an instance variable in cases like this.Asuncion
@GravityBringer: then "something" needs to involve a const_cast. It was intended to be a place-holder for the code required to get a non-const return from the const object, not as a place-holder for what would have been in the duplicated getter. So "something" is not just an instance variable.Roots
I see. That really diminishes the usefulness of the technique, though. I'd remove the downvote, but SO won't let me.Asuncion
This effectively achieves the same thing as Meyers, only that your const_cast is hidden in something. If something wouldn't have had to be const_casted, it would be because it couldn't be used to modify the object on which the method was called and then you wouldn't need any non-const version version of the method.Feldstein
@HelloGoodbye: that's not correct. It is not true that separate const and non-const versions are needed only if a const_cast is required. For example, something might boil down to *some_pointer_data_member (and suppose you want to return a const reference in the const case), in which case Meyers's code introduces a cast but you don't actually need one. His code provides a completely general pattern, but most actual functions that you write are more specific :-)Roots
Well, I guess in that case you could modify your own class instance even from a const method that's called on the instance, at least if the pointer member points to a member in the instance or to the instance itself, since the compiler doesn't know what the pointer is addressing. That didn't strike me until now.Feldstein
This is a very conceptual answer, this makes lots of sense when the returned reference is a pointer of the class.Intertidal
@HelloGoodbye, where is the hidden const_cast? If the returned thing is not const, the code is perfectly valid. The point of this answer is that it doesn't use const_cast implicit or explicit. It achieves the effect by simply breaking an usual convention, but it does it in its private implementation.Intertidal
C
8

You could also solve this with templates. This solution is slightly ugly (but the ugliness is hidden in the .cpp file) but it does provide compiler checking of constness, and no code duplication.

.h file:

#include <vector>

class Z
{
    // details
};

class X
{
    std::vector<Z> vecZ;

public:
    const std::vector<Z>& GetVector() const { return vecZ; }
    std::vector<Z>& GetVector() { return vecZ; }

    Z& GetZ( size_t index );
    const Z& GetZ( size_t index ) const;
};

.cpp file:

#include "constnonconst.h"

template< class ParentPtr, class Child >
Child& GetZImpl( ParentPtr parent, size_t index )
{
    // ... massive amounts of code ...

    // Note you may only use methods of X here that are
    // available in both const and non-const varieties.

    Child& ret = parent->GetVector()[index];

    // ... even more code ...

    return ret;
}

Z& X::GetZ( size_t index )
{
    return GetZImpl< X*, Z >( this, index );
}

const Z& X::GetZ( size_t index ) const
{
    return GetZImpl< const X*, const Z >( this, index );
}

The main disadvantage I can see is that because all the complex implementation of the method is in a global function, you either need to get hold of the members of X using public methods like GetVector() above (of which there always need to be a const and non-const version) or you could make this function a friend. But I don't like friends.

[Edit: removed unneeded include of cstdio added during testing.]

Calender answered 13/1, 2009 at 16:21 Comment(4)
You can always make the complex implementation function a static member to gain access to the private members. The function need only be declared in the the class header file, the definition can reside in the class implementation file. It is, after all, part of the class implementation.Kroon
Aah yes good idea! I don't like the template stuff appearing in the header, but if since here it potentially makes the implemtation quite a lot simpler it is probably worth it.Calender
+ 1 to this solution which doesn't duplicate any code, nor uses any ugly const_cast (which could accidentally be used to canst something that is actually supposed to be const to something that is not).Feldstein
Nowadays this can be simplified with a deduced return type for the template (especially useful since it reduces what has to be duplicated in the class in the member case).Bespread
I
8

For those (like me) who

  • use c++17
  • want to add the least amount of boilerplate/repetition and
  • don't mind using macros (while waiting for meta-classes...),

here is another take:

#include <utility>
#include <type_traits>

template <typename T> struct NonConst;
template <typename T> struct NonConst<T const&> {using type = T&;};
template <typename T> struct NonConst<T const*> {using type = T*;};

#define NON_CONST(func)                                                     \
    template <typename... T> auto func(T&&... a)                            \
        -> typename NonConst<decltype(func(std::forward<T>(a)...))>::type   \
    {                                                                       \
        return const_cast<decltype(func(std::forward<T>(a)...))>(           \
            std::as_const(*this).func(std::forward<T>(a)...));              \
    }

It is basically a mix of the answers from @Pait, @DavidStone and @sh1 (EDIT: and an improvement from @cdhowie). What it adds to the table is that you get away with only one extra line of code which simply names the function (but no argument or return type duplication):

class X
{
    const Z& get(size_t index) const { ... }
    NON_CONST(get)
};

Note: gcc fails to compile this prior to 8.1, clang-5 and upwards as well as MSVC-19 are happy (according to the compiler explorer).

Imprimatur answered 29/3, 2019 at 22:19 Comment(5)
This just worked straight-up for me. This is a great answer, thank you!Winser
Shouldn't the decltype()s also be using std::forward on the arguments to make sure that we're using right return type in the case where we have overloads of get() that take different types of references?Tui
@Tui Can you provide an example?Imprimatur
@Imprimatur It's contrived as hell, but here you go. The NON_CONST macro deduces the return type incorrectly and const_casts to the wrong type due to the lack of forwarding in the decltype(func(a...)) types. Replacing them with decltype(func(std::forward<T>(a)...)) solves this. (There's just a linker error because I never defined any of the declared X::get overloads.)Tui
Thanks @cdhowie, I pimped your example to actually use the non-const overloads: coliru.stacked-crooked.com/a/0cedc7f4e789479eImprimatur
C
6

If you don't like const casting, I use this C++17 version of the template static helper function suggested by another answer, with and optional SFINAE test.

#include <type_traits>

#define REQUIRES(...)         class = std::enable_if_t<(__VA_ARGS__)>
#define REQUIRES_CV_OF(A,B)   REQUIRES( std::is_same_v< std::remove_cv_t< A >, B > )

class Foobar {
private:
    int something;

    template<class FOOBAR, REQUIRES_CV_OF(FOOBAR, Foobar)>
    static auto& _getSomething(FOOBAR& self, int index) {
        // big, non-trivial chunk of code...
        return self.something;
    }

public:
    auto& getSomething(int index)       { return _getSomething(*this, index); }
    auto& getSomething(int index) const { return _getSomething(*this, index); }
};

Full version: https://godbolt.org/z/mMK4r3

Codie answered 5/12, 2019 at 18:23 Comment(0)
T
5

While most of answers here suggest to use a const_cast, CppCoreGuidelines have a section about that:

Instead, prefer to share implementations. Normally, you can just have the non-const function call the const function. However, when there is complex logic this can lead to the following pattern that still resorts to a const_cast:

class Foo {
public:
    // not great, non-const calls const version but resorts to const_cast
    Bar& get_bar()
    {
        return const_cast<Bar&>(static_cast<const Foo&>(*this).get_bar());
    }
    const Bar& get_bar() const
    {
        /* the complex logic around getting a const reference to my_bar */
    }
private:
    Bar my_bar;
};

Although this pattern is safe when applied correctly, because the caller must have had a non-const object to begin with, it's not ideal because the safety is hard to enforce automatically as a checker rule.

Instead, prefer to put the common code in a common helper function -- and make it a template so that it deduces const. This doesn't use any const_cast at all:

class Foo {
public:                         // good
          Bar& get_bar()       { return get_bar_impl(*this); }
    const Bar& get_bar() const { return get_bar_impl(*this); }
private:
    Bar my_bar;

    template<class T>           // good, deduces whether T is const or non-const
    static auto& get_bar_impl(T& t)
        { /* the complex logic around getting a possibly-const reference to my_bar */ }
};

Note: Don't do large non-dependent work inside a template, which leads to code bloat. For example, a further improvement would be if all or part of get_bar_impl can be non-dependent and factored out into a common non-template function, for a potentially big reduction in code size.

Tailing answered 23/6, 2021 at 12:42 Comment(0)
K
3

How about moving the logic into a private method, and only doing the "get the reference and return" stuff inside the getters? Actually, I would be fairly confused about the static and const casts inside a simple getter function, and I'd consider that ugly except for extremely rare circumstances!

Kiva answered 23/9, 2008 at 22:5 Comment(2)
In order to avoid undefined behavior you still need a const_cast. See the answer by Martin York and my comment there.Principal
Kevin, what answer by Martin YorkGaekwar
S
2

I'd suggest a private helper static function template, like this:

class X
{
    std::vector<Z> vecZ;

    // ReturnType is explicitly 'Z&' or 'const Z&'
    // ThisType is deduced to be 'X' or 'const X'
    template <typename ReturnType, typename ThisType>
    static ReturnType Z_impl(ThisType& self, size_t index)
    {
        // massive amounts of code for validating index
        ReturnType ret = self.vecZ[index];
        // even more code for determining, blah, blah...
        return ret;
    }

public:
    Z& Z(size_t index)
    {
        return Z_impl<Z&>(*this, index);
    }
    const Z& Z(size_t index) const
    {
        return Z_impl<const Z&>(*this, index);
    }
};
Sagamore answered 7/10, 2015 at 14:25 Comment(0)
A
2

Is it cheating to use the preprocessor?

struct A {

    #define GETTER_CORE_CODE       \
    /* line 1 of getter code */    \
    /* line 2 of getter code */    \
    /* .....etc............. */    \
    /* line n of getter code */       

    // ^ NOTE: line continuation char '\' on all lines but the last

   B& get() {
        GETTER_CORE_CODE
   }

   const B& get() const {
        GETTER_CORE_CODE
   }

   #undef GETTER_CORE_CODE

};

It's not as fancy as templates or casts, but it does make your intent ("these two functions are to be identical") pretty explicit.

Am‚lie answered 14/1, 2017 at 2:33 Comment(1)
But then you have to be careful with backslashes (as usual for multiline macros) and in addition you lose syntax highlighting in most (if not all) editors.Wherry
P
2

It's surprising to me that there are so many different answers, yet almost all rely on heavy template magic. Templates are powerful, but sometimes macros beat them in conciseness. Maximum versatility is often achieved by combining both.

I wrote a macro FROM_CONST_OVERLOAD() which can be placed in the non-const function to invoke the const function.

Example usage:

class MyClass
{
private:
    std::vector<std::string> data = {"str", "x"};

public:
    // Works for references
    const std::string& GetRef(std::size_t index) const
    {
        return data[index];
    }

    std::string& GetRef(std::size_t index)
    {
        return FROM_CONST_OVERLOAD( GetRef(index) );
    }


    // Works for pointers
    const std::string* GetPtr(std::size_t index) const
    {
        return &data[index];
    }

    std::string* GetPtr(std::size_t index)
    {
        return FROM_CONST_OVERLOAD( GetPtr(index) );
    }
};

Simple and reusable implementation:

template <typename T>
T& WithoutConst(const T& ref)
{
    return const_cast<T&>(ref);
}

template <typename T>
T* WithoutConst(const T* ptr)
{
    return const_cast<T*>(ptr);
}

template <typename T>
const T* WithConst(T* ptr)
{
    return ptr;
}

#define FROM_CONST_OVERLOAD(FunctionCall) \
  WithoutConst(WithConst(this)->FunctionCall)

Explanation:

As posted in many answers, the typical pattern to avoid code duplication in a non-const member function is this:

return const_cast<Result&>( static_cast<const MyClass*>(this)->Method(args) );

A lot of this boilerplate can be avoided using type inference. First, const_cast can be encapsulated in WithoutConst(), which infers the type of its argument and removes the const-qualifier. Second, a similar approach can be used in WithConst() to const-qualify the this pointer, which enables calling the const-overloaded method.

The rest is a simple macro that prefixes the call with the correctly qualified this-> and removes const from the result. Since the expression used in the macro is almost always a simple function call with 1:1 forwarded arguments, drawbacks of macros such as multiple evaluation do not kick in. The ellipsis and __VA_ARGS__ could also be used, but should not be needed because commas (as argument separators) occur within parentheses.

This approach has several benefits:

  • Minimal and natural syntax -- just wrap the call in FROM_CONST_OVERLOAD( )
  • No extra member function required
  • Compatible with C++98
  • Simple implementation, no template metaprogramming and zero dependencies
  • Extensible: other const relations can be added (like const_iterator, std::shared_ptr<const T>, etc.). For this, simply overload WithoutConst() for the corresponding types.

Limitations: this solution is optimized for scenarios where the non-const overload is doing exactly the same as the const overload, so that arguments can be forwarded 1:1. If your logic differs and you are not calling the const version via this->Method(args), you may consider other approaches.

Patroclus answered 20/6, 2019 at 22:9 Comment(0)
S
1

I came up with a macro that generates pairs of const/non-const functions automatically.

class A
{
    int x;    
  public:
    MAYBE_CONST(
        CV int &GetX() CV {return x;}
        CV int &GetY() CV {return y;}
    )

    //   Equivalent to:
    // int &GetX() {return x;}
    // int &GetY() {return y;}
    // const int &GetX() const {return x;}
    // const int &GetY() const {return y;}
};

See the end of the answer for the implementation.

The argument of MAYBE_CONST is duplicated. In the first copy, CV is replaced with nothing; and in the second copy it's replaced with const.

There's no limit on how many times CV can appear in the macro argument.

There's a slight inconvenience though. If CV appears inside of parentheses, this pair of parentheses must be prefixed with CV_IN:

// Doesn't work
MAYBE_CONST( CV int &foo(CV int &); )

// Works, expands to
//         int &foo(      int &);
//   const int &foo(const int &);
MAYBE_CONST( CV int &foo CV_IN(CV int &); )

Implementation:

#define MAYBE_CONST(...) IMPL_CV_maybe_const( (IMPL_CV_null,__VA_ARGS__)() )
#define CV )(IMPL_CV_identity,
#define CV_IN(...) )(IMPL_CV_p_open,)(IMPL_CV_null,__VA_ARGS__)(IMPL_CV_p_close,)(IMPL_CV_null,

#define IMPL_CV_null(...)
#define IMPL_CV_identity(...) __VA_ARGS__
#define IMPL_CV_p_open(...) (
#define IMPL_CV_p_close(...) )

#define IMPL_CV_maybe_const(seq) IMPL_CV_a seq IMPL_CV_const_a seq

#define IMPL_CV_body(cv, m, ...) m(cv) __VA_ARGS__

#define IMPL_CV_a(...) __VA_OPT__(IMPL_CV_body(,__VA_ARGS__) IMPL_CV_b)
#define IMPL_CV_b(...) __VA_OPT__(IMPL_CV_body(,__VA_ARGS__) IMPL_CV_a)

#define IMPL_CV_const_a(...) __VA_OPT__(IMPL_CV_body(const,__VA_ARGS__) IMPL_CV_const_b)
#define IMPL_CV_const_b(...) __VA_OPT__(IMPL_CV_body(const,__VA_ARGS__) IMPL_CV_const_a)

Pre-C++20 implementation that doesn't support CV_IN:

#define MAYBE_CONST(...) IMPL_MC( ((__VA_ARGS__)) )
#define CV ))((

#define IMPL_MC(seq) \
    IMPL_MC_end(IMPL_MC_a seq) \
    IMPL_MC_end(IMPL_MC_const_0 seq)

#define IMPL_MC_identity(...) __VA_ARGS__
#define IMPL_MC_end(...) IMPL_MC_end_(__VA_ARGS__)
#define IMPL_MC_end_(...) __VA_ARGS__##_end

#define IMPL_MC_a(elem) IMPL_MC_identity elem IMPL_MC_b
#define IMPL_MC_b(elem) IMPL_MC_identity elem IMPL_MC_a
#define IMPL_MC_a_end
#define IMPL_MC_b_end

#define IMPL_MC_const_0(elem)       IMPL_MC_identity elem IMPL_MC_const_a
#define IMPL_MC_const_a(elem) const IMPL_MC_identity elem IMPL_MC_const_b
#define IMPL_MC_const_b(elem) const IMPL_MC_identity elem IMPL_MC_const_a
#define IMPL_MC_const_a_end
#define IMPL_MC_const_b_end
Semaphore answered 19/10, 2019 at 17:43 Comment(0)
G
0

I did this for a friend who rightfully justified the use of const_cast... not knowing about it I probably would have done something like this (not really elegant) :

#include <iostream>

class MyClass
{

public:

    int getI()
    {
        std::cout << "non-const getter" << std::endl;
        return privateGetI<MyClass, int>(*this);
    }

    const int getI() const
    {
        std::cout << "const getter" << std::endl;
        return privateGetI<const MyClass, const int>(*this);
    }

private:

    template <class C, typename T>
    static T privateGetI(C c)
    {
        //do my stuff
        return c._i;
    }

    int _i;
};

int main()
{
    const MyClass myConstClass = MyClass();
    myConstClass.getI();

    MyClass myNonConstClass;
    myNonConstClass.getI();

    return 0;
}
Goofy answered 16/6, 2015 at 13:13 Comment(0)
R
-1

Typically, the member functions for which you need const and non-const versions are getters and setters. Most of the time they are one-liners so code duplication is not an issue.

Romanist answered 23/9, 2008 at 21:11 Comment(3)
That may be true most of the time. But there are exceptions.Principal
getters anyway, a const setter doesn't make much sense ;)Kozak
I meant that the non-const getter is effectively a setter. :)Romanist
J
-1

To add to the solution jwfearn and kevin provided, here's the corresponding solution when the function returns shared_ptr:

struct C {
  shared_ptr<const char> get() const {
    return c;
  }
  shared_ptr<char> get() {
    return const_pointer_cast<char>(static_cast<const C &>(*this).get());
  }
  shared_ptr<char> c;
};
Judaica answered 20/11, 2014 at 14:6 Comment(0)
E
-1

Didn't find what I was looking for, so I rolled a couple of my own...

This one is a little wordy, but has the advantage of handling many overloaded methods of the same name (and return type) all at once:

struct C {
  int x[10];

  int const* getp() const { return x; }
  int const* getp(int i) const { return &x[i]; }
  int const* getp(int* p) const { return &x[*p]; }

  int const& getr() const { return x[0]; }
  int const& getr(int i) const { return x[i]; }
  int const& getr(int* p) const { return x[*p]; }

  template<typename... Ts>
  auto* getp(Ts... args) {
    auto const* p = this;
    return const_cast<int*>(p->getp(args...));
  }

  template<typename... Ts>
  auto& getr(Ts... args) {
    auto const* p = this;
    return const_cast<int&>(p->getr(args...));
  }
};

If you have only one const method per name, but still plenty of methods to duplicate, then you might prefer this:

  template<typename T, typename... Ts>
  auto* pwrap(T const* (C::*f)(Ts...) const, Ts... args) {
    return const_cast<T*>((this->*f)(args...));
  }

  int* getp_i(int i) { return pwrap(&C::getp_i, i); }
  int* getp_p(int* p) { return pwrap(&C::getp_p, p); }

Unfortunately this breaks down as soon as you start overloading the name (the function pointer argument's argument list seems to be unresolved at that point, so it can't find a match for the function argument). Although you can template your way out of that, too:

  template<typename... Ts>
  auto* getp(Ts... args) { return pwrap<int, Ts...>(&C::getp, args...); }

But reference arguments to the const method fail to match against the apparently by-value arguments to the template and it breaks. Not sure why.Here's why.

Edla answered 15/12, 2016 at 4:15 Comment(0)
H
-2

This DDJ article shows a way using template specialization that doesn't require you to use const_cast. For such a simple function it really isn't needed though.

boost::any_cast (at one point, it doesn't any more) uses a const_cast from the const version calling the non-const version to avoid duplication. You can't impose const semantics on the non-const version though so you have to be very careful with that.

In the end some code duplication is okay as long as the two snippets are directly on top of each other.

Hydroxyl answered 23/9, 2008 at 20:53 Comment(1)
The DDJ article seems to refer to iterators -- which isn't relevant to the question. Const-iterators are not constant data -- they are iterators that point to constant data.Principal

© 2022 - 2024 — McMap. All rights reserved.