Does it ever make sense to check if "this" is null?

Question

Say I have a class with a member function; inside that method, I check this == nullptr, and if it is, return an error code.

If this is null, then that means the object is deleted. Is the method even able to return anything?

Update: I forgot to mention that the method can be called from multiple threads and it may cause the object to be deleted while another thread is inside the member function.

Ancilla

It doesn't mean the object was deleted. Deleting a pointer doesn't automatically zero it out, and ((Foo*)0)->foo() is perfectly valid syntax. As long as foo() is not a virtual function, this will even work on most compilers, but it's just icky. – Widthwise

"may cause the object to be deleted while another thread is inside the method". This is not acceptable, you must not delete an object while other code (in the same thread or a different one) retains a reference which it will use. It also won't cause this to become null in that other thread. – Musick

I am looking at a situation right now where this is definitely NULL. God only knows why. – Maladminister

"this" seems to be null whenever the parent class is abstract. – Maladminister

From GCC 6.2 release notes: Value range propagation now assumes that the this pointer of C++ member functions is non-null. – Maharashtra

Answer 1

Does it ever make sense to check for this==null? I found this while doing a code review.

In standard C++, it does not, because any call on a null pointer is already undefined behavior, so any code relying on such checks is non-standard (there's no guarantee that the check will even be executed).

Note that this holds true for non-virtual functions as well.

Some implementations permit this==0, however, and consequently libraries written specifically for those implementations will sometimes use it as a hack. A good example of such a pair is VC++ and MFC - I don't recall the exact code, but I distinctly remember seeing if (this == NULL) checks in MFC source code somewhere.

It may also be there as a debugging aid, because at some point in the past this code was hit with this==0 because of a mistake in the caller, so a check was inserted to catch future instances of that. An assert would make more sense for such things, though.

If this == null then that means the object is deleted.

No, it doesn't mean that. It means that a method was called on a null pointer, or on a reference obtained from a null pointer (though obtaining such a reference is already U.B.). This has nothing to do with delete, and does not require any objects of this type to have ever existed.

Rationalism

With delete, the opposite is usually true -- if you delete an object, it doesn't get set to NULL, and then you attempt to call a method on it, you'll find that this != NULL, but it will likely crash or behave oddly if its memory has already been reclaimed for use by some other object. – Onanism

IIRC, the standard allows for delete to set the pointer to NULL, but does not require it, and in practice almost no implementations do so. – Pekingese

delete might not always get an l-value though, consider delete this + 0; – Physiology

Also, in C++ new is supposed to throw an exception on failure, not to return NULL (although many compilers including MSVC up to version 6 return NULL when new fails). – Antislavery

Visual Studio 6 was released in June 1998. The C++ standard was published in September. OK, so Microsoft could have anticipated the standard, but in principle it is not surprising that many pre-standard compilers do not implement the standard ;-) – Musick

@Steve: The problem was you couldn't write standard-conforming C++ with msvc6 and that it was still used for so long after '98. – Evetta

Just for fun, here is a nice article about MFC and it's use of if (this == 0) viva64.com/en/b/0226 – Lemonade

Note that MFC is a bit of a special case because it was never really intended to be used with anything other than VC++, and because the teams were in contact, the MFC team could rely on implementation details like this (and ensure that they'll be around for as long as needed, and behave exactly as desired). Since this behavior is not otherwise documented or guaranteed even for VC++, third-party libraries cannot rely on it. – Rationalism

Answer 2

Your note about threads is worrisome. I'm pretty sure you have a race condition that can lead to a crash. If a thread deletes an object and zeros the pointer, another thread could make a call through that pointer between those two operations, leading to this being non-null and also not valid, resulting in a crash. Similarly, if a thread calls a method while another thread is in the middle of creating the object, you may also get a crash.

Short answer, you really need to use a mutex or something to synchonize access to this variable. You need to ensure that this is never null or you're going to have problems.

Widthwise

"You need to ensure that this is never null" - I think a better approach is to ensure that the left operand of operator-> is never null :) but aside from that, I wish I could +10 this. – Rationalism

Couldnt he use thread synchronization techniques? – Epencephalon

@Epencephalon Yes, "thread synchronization techniques" is a more proper way to say "a mutex or something." – Widthwise

Answer 3

I know that this is old but I feel like now that we're dealing with C++11-17 somebody should mention lambdas. If you capture this into a lambda that is going to be called asynchronously at a later point in time, it is possible that your "this" object gets destroyed before that lambda is invoked.

i.e passing it as a callback to some time-expensive function that is run from a separate thread or just asynchronously in general

EDIT: Just to be clear, the question was "Does it ever make sense to check if this is null" I am merely offering a scenario where it does make sense that might become more prevalent with the wider use of modern C++.

Contrived example: This code is completely runable. To see unsafe behavior just comment out the call to safe behavior and uncomment the unsafe behavior call.

#include <memory>
#include <functional>
#include <iostream>
#include <future>

class SomeAPI
{
public:
    SomeAPI() = default;

    void DoWork(std::function<void(int)> cb)
    {
        DoAsync(cb);
    }

private:
    void DoAsync(std::function<void(int)> cb)
    {
        std::cout << "SomeAPI about to do async work\n";
        m_future = std::async(std::launch::async, [](auto cb)
        {
            std::cout << "Async thread sleeping 10 seconds (Doing work).\n";
            std::this_thread::sleep_for(std::chrono::seconds{ 10 });
            // Do a bunch of work and set a status indicating success or failure.
            // Assume 0 is success.
            int status = 0;
            std::cout << "Executing callback.\n";
            cb(status);
            std::cout << "Callback Executed.\n";
        }, cb);
    };
    std::future<void> m_future;
};

class SomeOtherClass
{
public:
    void SetSuccess(int success) { m_success = success; }
private:
    bool m_success = false;
};
class SomeClass : public std::enable_shared_from_this<SomeClass>
{
public:
    SomeClass(SomeAPI* api)
        : m_api(api)
    {
    }

    void DoWorkUnsafe()
    {
        std::cout << "DoWorkUnsafe about to pass callback to async executer.\n";
        // Call DoWork on the API.
        // DoWork takes some time.
        // When DoWork is finished, it calls the callback that we sent in.
        m_api->DoWork([this](int status)
        {
            // Undefined behavior
            m_value = 17;
            // Crash
            m_data->SetSuccess(true);
            ReportSuccess();
        });
    }

    void DoWorkSafe()
    {
        // Create a weak point from a shared pointer to this.
        std::weak_ptr<SomeClass> this_ = shared_from_this();
        std::cout << "DoWorkSafe about to pass callback to async executer.\n";
        // Capture the weak pointer.
        m_api->DoWork([this_](int status)
        {
            // Test the weak pointer.
            if (auto sp = this_.lock())
            {
                std::cout << "Async work finished.\n";
                // If its good, then we are still alive and safe to execute on this.
                sp->m_value = 17;
                sp->m_data->SetSuccess(true);
                sp->ReportSuccess();
            }
        });
    }
private:
    void ReportSuccess()
    {
        // Tell everyone who cares that a thing has succeeded.
    };

    SomeAPI* m_api;
    std::shared_ptr<SomeOtherClass> m_data = std::shared_ptr<SomeOtherClass>();
    int m_value;
};

int main()
{
    std::shared_ptr<SomeAPI> api = std::make_shared<SomeAPI>();
    std::shared_ptr<SomeClass> someClass = std::make_shared<SomeClass>(api.get());

    someClass->DoWorkSafe();

    // Comment out the above line and uncomment the below line
    // to see the unsafe behavior.
    //someClass->DoWorkUnsafe();

    std::cout << "Deleting someClass\n";
    someClass.reset();

    std::cout << "Main thread sleeping for 20 seconds.\n";
    std::this_thread::sleep_for(std::chrono::seconds{ 20 });

    return 0;
}

Echelon

But even if the object is destroyed, won't the lambda end up with a dangling non-null captured this pointer? – Irvine

Precisely! Checking if "this" == nullptr or NULL will not be sufficient in this case as "this" will be dangling. I was merely just mentioning it since some were skeptical of such a semantic even needing to exist. – Echelon

I don't think this answer is relevant as the question only concerns "Say I have a class with a method; inside that method". Lambdas are only one example of where you can get a dangling pointer. – Maharashtra

"Does it ever make sense to check if this is null" was the question. I was merely offering a scenario that might become more prevalent with the wider use of modern C++. – Echelon

This answer is extremely useful, and I think it belongs in this question's thread as it addresses a specific case in which we should check if this is null. – Palomino

Can you clarify how testing this==nullptr improves robustness here? Given that this is dangling and not null, it seems to be irrelevant. – Bailable

Answer 4

FWIW, I have used debugging checks for (this != NULL) in assertions before which have helped catch defective code. Not that the code would have necessarily gotten too far with out a crash, but on small embedded systems that don't have memory protection, the assertions actually helped.

On systems with memory protection, the OS will generally hit an access violation if called with a NULL this pointer, so there's less value in asserting this != NULL. However, see Pavel's comment for why it's not necessarily worthless on even protected systems.

Bathypelagic

There is still a case for asserts, AV or not. The problem is that an AV will usually not happen until a member function actually tries to access some member field. Quite often they just call something else (etc...), until eventually something crashes down the line. Alternatively, they can call a member of another class or a global function, and pass the (supposedly non-null) this as an argument. – Rationalism

@Pavel: true enough - I softened my wording about the value of asserting this on systems with memory protection. – Bathypelagic

actually, if assert is going to work, then definitely the main code also will work right? – Md

@Gokul: I'm not sure I fully understand your question, but even if the assert 'passes' you could get a bogus this pointer if your program is doing bad things with pointer arithmetic or maybe calling a member function of a class for an object composed inside embedded in another class through a NULL pointer for the 'outer' class. I hope that sentence made some sort of sense. – Bathypelagic

From my point of view, you said something that is not logical. The OS has nothing to do with arguments you send to functions in your application. – Epencephalon

Answer 5

On at least one compiler, yes, the check makes sense. It will work as you expect and it is possible to trigger it. Whether it’s useful is questionable, since well-formed code should never call a member function on a null pointer, but assert(this); is cheap.

The following code will compile on MSVC 19.37 and run as expected. It will not issue a warning, even with /std:c++20 /Wall /external:anglebrackets /external:W0.

#include <cstdlib>
#include <functional>
#include <iostream>

using std::cerr, std::cout;

class Foo {
public:
    void sillyContrivance() const noexcept {
        if (this) {
            cout << "hello, world!\n";
        } else {
            cerr << "Null this pointer!\n";
        }
     }
};

int main() {
    static_cast<Foo*>(nullptr)->sillyContrivance();
    const auto closure = std::bind(&Foo::sillyContrivance, static_cast<Foo*>(nullptr));
    closure();
}

The program prints Null this pointer! twice.

Clang 16.0.0 will warn you that this cannot be null, turn the check into a no-op, and print hello, world! twice. GCC 13.2 will additionally warn you that you are calling a member function on a null pointer, and also print hello, world! twice.

In real-world, practical use, a member function that never needs to dereference this would have been declared static, so realistic code compiled with Clang or GCC that triggers this bug (such as passing around a default-initialized struct containing an object pointer) would segfault on modern OSes. However, the sanity check would be useless on compilers that optimize it away.

Bruns

Answer 6

Your method will most likely (may vary between compilers) be able to run and also be able to return a value. As long as it does not access any instance variables. If it tries this it will crash.

As others pointed out you can not use this test to see if an object has been deleted. Even if you could, it would not work, because the object may be deleted by another thread just after the test but before you execute the next line after the test. Use Thread synchronization instead.

If this is null there is a bug in your program, most likely in the design of your program.

Dagmardagna

Answer 7

I'd also add that it's usually better to avoid null or NULL. I think the standard is changing yet again here but for now 0 is really what you want to check for to be absolutely sure you're getting what you want.

Travail

Checking for NULL does no such thing. – Adai

Answer 8

This is just a pointer passed as the first argument to a function (which is exactly what makes it a method). So long as you're not talking about virtual methods and/or virtual inheritance, then yes, you can find yourself executing an instance method, with a null instance. As others said, you almost certainly won't get very far with that execution before problems arise, but robust coding should probably check for that situation, with an assert. At least, it makes sense when you suspect it could be occuring for some reason, but need to track down exactly which class / call stack it's occurring in.

Amherst

Answer 9

I know this is a old question, however I thought I will share my experience with use of Lambda capture

#include <iostream>
#include <memory>

using std::unique_ptr;
using std::make_unique;
using std::cout;
using std::endl;

class foo {
public:
    foo(int no) : no_(no) {

    }

    template <typename Lambda>
    void lambda_func(Lambda&& l) {
        cout << "No is " << no_ << endl;
        l();
    }

private:
    int no_;
};

int main() {
    auto f = std::make_unique<foo>(10);

    f->lambda_func([f = std::move(f)] () mutable {
        cout << "lambda ==> " << endl;
        cout << "lambda <== " << endl;
    });

    return 0;
}

This code segment faults

$ g++ -std=c++14  uniqueptr.cpp  
$ ./a.out 
Segmentation fault (core dumped)

If I remove the std::cout statement from lambda_func The code runs to completion.

It seems like, this statement f->lambda_func([f = std::move(f)] () mutable { processes lambda captures before member function is invoked.

Interrogatory