Compiler warning on "Effective C++"s method to avoid duplication in const and non-const member functions

Question

This question has been updated. Please review the code.

The following code was compiled with VC++ Nov 2012 CTP. Scott Meyers' book "Effective C++" recommend that we should use the method of to avoid duplication in const and non-const member functions. However, the following code cause a warning (level 1). Because WDK build tool treats warnings as errors, so the following code cannot be compiled successfully.

Is there other better method?

struct A
{
    int n;

    A(int n)
        : n(n)
    {}

    int Get() const
    {
        return n;
    }

    int Get()
    {
        return static_cast<const decltype(*this)&>(*this).Get();
    }
};

int main()
{
    const A a(8);

    //
    // warning C4717: 'A::Get' : recursive on all control paths,
    // function will cause runtime stack overflow
    //
    a.Get(); 
}

Not sure it is related in any way to the warning, but conceptually your `const` version should be `const int& Get() const`, i.e. returning reference to const. — juanchopanza, Feb 28 '13 at 09:38
In this particular case, the "duplication" would be `return n;`. How is this an improvement on that? We see that introducing extra code might sometimes also cause extra errors... — Bo Persson, Feb 28 '13 at 10:00
This is a bad example because you actually don't need the non-const Get here. Apart from that, decltype does not behave like you expected. You're not really adding a const this way, so you have an endlessly recursive call. `decltype(*this)` yields A& in your case and since references are not objects adding const will be ignored. — sellibitze, Feb 28 '13 at 11:39
@xmllmx - I don't think Scott Meyers actually recommends avoiding duplication *at any cost*, he is just offering an alternative for some cases. If avoiding repetition is the goal, and not the means, you can just use `return n + 0;` and `return n - 0;` to make the code different. — Bo Persson, Feb 28 '13 at 12:00

score 6 · Accepted Answer · answered Feb 28 '13 at 09:42

6

You've transposed the bodies of the two Get methods, so the compiler is correct; the const Get method is calling itself. Aren't you glad now that your build tool treats warnings as errors? :)

Swap them round:

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

const int& Get() const
{
    return n;
}

answered Feb 28 '13 at 09:42

ecatmur

152,476
27
293
366

Thus the compiler warning is valid. – ta.speot.is Feb 28 '13 at 09:45

score 4 · Answer 2 · edited May 23 '17 at 11:59

4

I believed you got it reverse. This is the non-const version which casts away constness on the const version.

See: Elegant solution to duplicate, const and non-const, getters?

edited May 23 '17 at 11:59

Community

1
1

answered Feb 28 '13 at 09:43

fjardon

7,921
22
31

score 1 · Answer 3 · answered Feb 28 '13 at 11:50

Answering the updated question. (You should have made this a new question)

In static_cast<const decltype(*this)&>(*this), *this is an lvalue of type A, so the type denoted by decltype(*this) is A& (see 7.1.6.2 [dcl.type.simple]/4).

As a result your non-const Get() function is equivalent to:

int Get()
{
    typedef A & Aref;
    // IMHO a const_cast would be the better choice here
    return static_cast<const Aref&>(*this).Get();
}

cv-qualifiers on a reference type are ignored. With reference-collapsing, your cast is ultimately equvalent to static_cast<A&>(*this), so you don't add the const you need.

So using decl_typedoes not work here. If you very badly want to use it, you'd need:

int Get()
{
    // Made my suggested change of cast here
    return const_cast<const std::remove_reference<decltype(*this)>::type&>(*this).Get();
}

Yes. I did. See http://stackoverflow.com/questions/15133644/why-does-decltypethis-not-return-the-correct-type — xmllmx, Feb 28 '13 at 11:53

Compiler warning on "Effective C++"s method to avoid duplication in const and non-const member functions

3 Answers3

Linked