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.

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.

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.

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;
}

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.

Answer 5

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.

Answer 6

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.

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.

Answer 8

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.