Return pointer-to-member in const method

SimonSchroeder

Maybe to answer a few questions (as short as possible).

Yes, it is good practice to overload your methods for const, just as you described:

int *pointerToMember() { return &member; }
const int *pointerToMember() const { return &member; }

The problem to reimplementing the const and non-const version is quite old. The standard book on these kind of problems is "Effective C++" by Scott Meyers. I found these answers on StackOverflow referencing this book for this problem:
https://stackoverflow.com/questions/856542/elegant-solution-to-duplicate-const-and-non-const-getters
https://stackoverflow.com/questions/123758/how-do-i-remove-code-duplication-between-similar-const-and-non-const-member-func/123995
How to select on implementation over the other? If you put const after a method declaration like this:
const int *pointerToMember() const { return &member; }
it means that the this pointer is const. This is why you should use const correctness throughout your entire program. Then you don't have to think about which version you should select. If your object (or pointer/reference to object) is const, you can only call const-method and thus never change the object. If your object (or pointer/reference to object) is non-const, it has the right to change. This means the following for your control:

Foo &o1 = getObjectFromSomewhere();  // non-const object => changes allowed
o1->pointerToMember();               // o1 is non-const => this-pointer to pointerToMember() is non-const
                                     // => call non-const method
const Foo &o2 = getObjectFromSomewhere(); // I know I don't want to change anything => get only const-reference
o2->pointerToMember();                    // o2 is const => this-pointer to pointerToMember() is const
                                          // => call to const method

// force const method for o1 as well
const_cast<const Foo&>(o1)->pointerToMember();

I guess this would be proper C++. I tend to write const as often as possible and only leave it out if I want to change an object.

I suggest reading Scott Meyers' books on effective C++.

Chris Kawa

@SimonSchroeder said:

const int *pointerToMember() const { return &member; }
it means that the this pointer is const

No, it's not ;) The object it points to is const. As I mentioned earlier the pointer itself is not.

// force const method for o1 as well
const_cast<const Foo&>(o1)->pointerToMember();

A more semantic (and shorter) way of writing this in modern C++ is using std::as_const or qAsConst in Qt, which do the same thing, just doesn't look as hacky.

JonB

@SimonSchroeder
I read the two links on stackoverflow. Both of them, and that guy's book, came up with what I have come to from @jsulm's solution above:

int *pointerToMember()  { const MyClass *_this = this;  return const_cast<int*>(_this->pointerToMember()); } 
// or
int *pointerToMember()  { return const_cast<int*>( const_cast<const MyClass *>(this)->pointerToMember() ); }

So I am a happy bunny, within the bounds of C++ obscure-readability :)

JonB

Dear @jsulm
I am now having to unmark your proposal of:

const int *pointerToMember() const { return &member; }
int *pointerToMember()  { const MyClass *_this = this;  return const_cast<int*>(_this->pointerToMember()); } // Now compiler knows that you want to call const pointerToMember

as acceptable here. All because of https://forum.qt.io/topic/120489/qvector-one-line-deep-copy/16, where it turned out to cause me horrible grief :)

My situation is like:

const Class::MyStruct *Class::find(int arg) const
{
    for (const MyStruct &ms : current)
        if (ms.arg== arg)
            return &ms;
    return nullptr;
}

Class::MyStruct *Class::find(int arg)
{
    const Class *_this = this;
    return const_cast<MyStruct *>(_this->find(arg));
}

QVector<MyStruct> current, saved;  // member variables
current.append(...);  // this can be called at various times
saved = current;  // this can be called at various times

MyStruct *ms = find(something);  // this will be found in current
if (ms != nullptr)
    ms->someMember = newValue;  // want to change in current, only
        // but it doesn't, it *also* means it has changed in saved too
        // because this fails to cause a "copy-on-write"
        // as a consequence (apparently) of the const_cast<> in the "writeable" find()

So my actual pointerToMember() needs to return a pointer to an element in a member QVector. That must be allowed, but your proposal "breaks" Qt's shared-value copy-on-write behaviour, as described in the other thread.

So now what do you propose for a "safe" solution here? :)

Christian Ehrlicher

@JonB said in Return pointer-to-member in const method:

So now what do you propose for a "safe" solution here?

Implement the non-const version and call it in the const version (but may lead to an unneeded detach)
implement it twice
don't use a cow container
use a template:

struct s
{
    int one = 1;
    int two = 2;
};

class foo
{
public:
    s* getFoo(int idx) { return getFooInternal<s*>(this, idx); }
    const s* getFoo(int idx) const { return getFooInternal<const s*>(this, idx); }
private:
    template <typename T, typename F>
    static T getFooInternal(F *f, int idx)
    {
        return &f->m_foo[idx];
    }
    QVector<s> m_foo;
};

JonB

@Christian-Ehrlicher said in Return pointer-to-member in const method:

Implement the non-const version and call it in the const version (but may lead to an unneeded detach)

Yes, I will think about that. I never cared about copy-on-write in this function's case.

implement it twice

LOL. That's not a solution, it's a workaround! You saw my lookup code, I'm not duplicating that!

don't use a cow container

No cows anywhere in my code....? Oohhhh, sorry, got it...

use a template:

I will indeed look at your code tomorrow, I had a feeling templates might come into it.

TBH, all I really want here, when I think about, is to be allowed to call a non-const member method from a const member function, in this case. My non-const function doesn't alter anything --- only returns a pointer to internal which might be used to write into by caller. But I won't be doing any such thing when calling from a const member. I (think I) want a new semi_const keyword, at least for a method, which does just promise not to alter the state of *this. That's all I thought const method() did when I started this thread. Is that so much to ask for? :)

Christian Ehrlicher

Simplified it the template little bit more:

class foo
{
    template <typename F>
    static auto getFooInternal(F *f, int idx)
    {
        return &f->m_foo[idx];
    }
public:
    s* getFoo(int idx) { return getFooInternal(this, idx); }
    const s* getFoo(int idx) const { return getFooInternal(this, idx); }
private:
    QVector<s> m_foo;
};

SimonSchroeder

@JonB said in Return pointer-to-member in const method:

MyStruct *ms = find(something);  // this will be found in current
if (ms != nullptr)
    ms->someMember = newValue;  // want to change in current, only
        // but it doesn't, it *also* means it has changed in saved too
        // because this fails to cause a "copy-on-write"
        // as a consequence (apparently) of the const_cast<> in the "writeable" find()

I believe this is not how copy-on-write for QVector works. (Can someone back me up or correct me on this?) I don't know of any mechanism in C++ which would allow to monitor changes to memory. ms->someMember = newValue; will not, in my understanding, trigger a copy of the vector. Appending, inserting, removing, etc. will trigger a copy. I am not certain if operator[](int) without const would trigger a copy. In this case you should implement Class::find twice because then for(const MyStruct &ms : current) and for(Mystruct &ms : current) would behave differently.

I would usually return a reference instead of the pointer. Then, the template trick by @Christian-Ehrlicher would help:

template<class T>
T Class::find(int arg) // has to be static
{
    for(T ms : current)
    ...
}

Your implementations could then call find<const MyStruct&>(arg) and find<MyStruct&>(arg). With a small change, this also works with your pointer:

template<class T>
T *Class:find(int arg) // still static
{
    for(T &ms : current)
    ...
}

// calls:
find<const MyStruct>(arg);
find<MyStruct>(arg);

Furthermore, it is very common to have a QVector<MyStruct*> instead of QVector<MyStruct>. This will further decouple copy-on-write for the QVector. The major reason to store a pointer instead of the object is to still have polymorphism and being able to have inherited objects inside your QVector, as well. Another reason would be if your objects are quite large. Expanding the vector would be slower because the whole objects instead of just pointers would need to be copied.

JonB

@SimonSchroeder
Hi Simon,

Several points from you, thank you, let me address a couple of them.

I believe this is not how copy-on-write for QVector works. (Can someone back me up or correct me on this?) I don't know of any mechanism in C++ which would allow to monitor changes to memory. ms->someMember = newValue; will not, in my understanding, trigger a copy of the vector.

I never said it would! It doesn't. What I said was

as a consequence (apparently) of the const_cast<> in the "writeable" find()

You have to remember the find() method did its work my moving through current by reference. I expected the CoW to have occurred during that, then my assignment would have only affected current. But it didn't CoW. And the reason for that is in the two definitions of find() given to me by Mr @jsulm. Which I liked, and thought would work, but fails in this situation. It would have worked if the "writeable" definition went for (MyStruct &ms : current), while the "read-only" one went for (const MyStruct &ms : current). But because instead it uses only the latter, const one to search, and then goes return const_cast<MyStruct *>(_this->find(arg));, this breaks my expected CoW.

In fact, if as @Christian-Ehrlicher said earlier:

Implement the non-const version and call it in the const version (but may lead to an unneeded detach)

I reverse code, so that the "writeable" one does for (MyStruct &ms : current) (which will CoW) and make the read-only one call that, it does work. But, as he observes, that is inefficient insofar as it CoWs everything even in the read-only case.

I am not certain if operator[](int) without const would trigger a copy

It does. I stated that even just putting a watch on current[something] in the Debugger pane is enough to trigger the copy!

In this case you should implement Class::find twice because then for(const MyStruct &ms : current) and for(Mystruct &ms : current) would behave differently.

Yes, that makes it work, but that is what I am asking to avoid! Personally --- maybe not you --- I am not happy implementing a method body twice --- it can be quite a few lines of code --- in order to deal with the vagaries of const. It leads to code-bloat and potential maintenance/bug problems. The algorithm is identical, I should not "have to" write two definitions to keep const happy. Just my 2 cents. But it's what I am interested in the question.

I would usually return a reference instead of the pointer.

As I wrote earlier, the need for pointer and not reference is that the find() absolutely can fail to find the match, and so has to be able to return nullptr, which is why I wrote it that way. Tell me how a reference solution allows for that?

FTR: At the time I wrote the find() there was no second copy/reference to the vector. Everything worked fine. Only as I expanded and found I needed a separate copy did the CoW problem rear its head.

Christian Ehrlicher

Use my template, it's working as expected :)