For a strictly internal class that is not intended to be used as part of an API provided to an external client, is there anything inherently evil with initializing a class pointer member variable to itself rather than NULL or nullptr?

Please see the below code for an example.

#include <iostream>

class Foo
{
public:
  Foo() :
    m_link(this)
  {
  }

  Foo* getLink()
  {
    return m_link;
  }

  void setLink(Foo& rhs)
  {
    m_link = &rhs;
    // Do other things too.
    // Obviously, the name shouldn't be setLink() if the real code is doing multiple things,
    // but this is a code sample.
  }

  void changeState()
  {
    // This is a code sample, but play along and assume there are actual states to change.
    std::cout << "Changing a state." << std::endl;
  }

private:
  Foo* m_link;
};

void doSomething(Foo& foo)
{
  Foo* link = foo.getLink();

  if (link == &foo)
  {
    std::cout << "A is not linked to anything." << std::endl;
  }

  else
  {
    std::cout << "A is linked to something else. Need to change the state on the link." << std::endl;
    link->changeState();
  }
}

int main(int argc, char** argv)
{
  Foo a;
  doSomething(a);

  std::cout << "-------------------" << std::endl;

  // This is a mere code sample.
  // In the real code, I'm fetching B from a container.
  Foo b;
  a.setLink(b);
  doSomething(a);

  return 0;
}

Output

A is not linked to anything.
-------------------
A is linked to something else. Need to change the state on the link.
Changing a state.

Pros

The benefit to initializing the pointer variable, Foo::link, to itself is to avoid accidental NULL dereferences. Since the pointer can never be NULL, then at worst, the program will produce erroneous output rather than segmentation fault.

Cons

However, the clear downside to this strategy is that it appears to be unconventional. Most programmers are used to checking for NULL, and thus don't expect to check for equality with the object invoking the pointer. As such, this technique would be ill-advised to use in a codebase that is targeted for external consumers, that is, developers expecting to use this codebase as a library.

Final Remarks

Any thoughts from anyone else? Has anyone else said anything substantial on this subject, especially with C 98 in consideration? Note that I compiled this code with a GCC compiler with these flags: -std=c 98 -Wall and did not notice any issues.

P.S. Please feel free to edit this post to improve any terminology I used here.

Edits

• This question is asked in the spirit of other good practice questions, such as this question about deleting references.
• A more extensive code example has been provided to clear up confusion. To be specific, the sample is now 63 lines which is an increase from the initial 30 lines. Thus, the variable names have been changed and therefore comments referencing Foo:p should apply to Foo:link.

CodePudding user response：

It's a bad idea to start with, but a horrendous idea as a solution to null dereferences.

You don't hide null dereferences. Ever. Null dereferences are bugs, not errors. When bugs happens, all invariances in your program goes down the toilet and there can be no guarantee for any behaviour. Not allowing a bug to manifest itself immediately doesn't make the program correct in any sense, it only serves to obfuscate and make debugging significantly more difficult.

That aside, a structure pointing into itself is a gnarly can of worms. Consider your copy assignment

Foo& operator=(const Foo& rhs) {
    if(this != &rhs)
        return *this;
    if(rhs->m_link != &rhs)
        m_link = this;
    else
        m_link = rhs->m_link;
}

You now have to check whether you're pointing to yourself every time you copy because its value is possibly tied to its own identity.

As it turns out, there's plenty of cases where such checks are required. How is swap supposed to be implemented?

void swap(Foo& x, Foo& y) noexcept {
    Foo* tx, *ty;
    if(x.m_link == &x)
        tx = &y;
    else
        tx = x.m_link;
    if(y.m_link == &y)
        ty = &x;
    else
        ty = y.m_link;

    x.m_link = ty;
    y.m_link = tx;
}

Suppose Foo has some sort of pointer/reference semantics, then your equality is now also non-trivial

bool operator==(const Foo& rhs) const {
    return m_link == rhs.m_link || (m_link == this && rhs.m_link == &rhs);
}

Don't point into yourself. Just don't.

CodePudding user response：

Foo is responsible for its own state. Especially pointers it exposes to its users.

If you expose a pointer in this fashion, as a public member, it is a very odd design decision. My gut has told me the last 30 odd years a pointer like this is not a responsible way to handle Foo's state.

Consider providing getters for this pointer instead.

Foo* getP() {
    // create a safe pointer for user
    // and indicate an error state. (exceptions might be an alternative)
}

Unless you share more context what Foo is, advice is hard to provide.

CodePudding user response：

is there anything inherently evil with initializing a class pointer member variable to itself rather than NULL or nullptr?

No. But as you pointed out, there might be different considerations depending on the use case.

I'm not sure this would be relevant under most circumstances, but there are some instances where an object needs to hold a pointer of its own type, so its really just pertinent to those cases.

For instance, an element in a singly-linked list will have a pointer to the next element, so the last element in the list would normally have a NULL pointer to show there are no further elements. So using this example, the end element could instead point to itself instead of NULL to denote it is the last element. It really just depends on personal implementation preference.

Many times, you can end up obfuscating code needlessly when trying too hard to make it crash-proof. Depending on the situation, you might mask issues and make problems much harder to debug. For instance, going back to the singly-linked example, if the pointer-to-self initialization method is used, and a bug in the program attempts to access the next element from the end element in the list, the list will return the end element again. This would most likely cause the program to continue "traversing" the list for eternity. That might be harder to find/understand than simply letting the program crash and finding the culprit via debugging tools.