Is it good practice to have your C++/Qt functions always check all its arguments for null values?

Question

Backstory

While developing with Qt Signal/Slots, I came across a few segmentation faults that had me puzzled as to what was causing it. Eventually I figured out that you could actually pass a slot function without providing it any of its arguments, which of course would cause a Segmentation Fault if the argument was used.

To avoid this pitfall, I am getting into the habit of checking all my arguments to make sure they are not NULL:

void MyClass::setMyString(QString input)
{
    if (input.isNull()) {
        qDebug() << "Error: setMyString() received NULL argument";
        return;
    }

    m_MyString = input;
    emit myStringChanged();
}

I'm thinking I ought to do this with all my functions from now on. Part of the reasoning is that I figure that it can not hurt to do this, and that if I or someone else ever needs to re-factor my code, it is less likely they will omit a necessary null value check.

Questions:

1. Is this an acceptable style in all or most circumstances?
2. Should I be introducing a runtime error, halting the program, instead of simply returning the function?
3. Does this style of coding improve security?
4. Does this style of coding improve stability?
5. Are there any performance considerations, to the extent that checking for null values may take longer depending on the type?
6. Beyond c++ and Qt, what other languages benefit from this style?
7. Are there any significant drawbacks worth considering?

Answer 1

Kind of echoing Basile here but with a slightly more negative tone. I'd say "yes" too, but be careful with that if you are allowing silent violations of preconditions. As a personal example, I worked in a C codebase that did this kind of stuff through an API:

int f(struct Foo* foo)
{
    if (!foo)
        return error;
    ...
    return success;
}

Perhaps even with a debug log in the middle. Yet it was a massive codebase, tens of millions of lines of code, over 30 years of development, and with pretty shoddy standards. The warnings in places like logs were filled to the brim with developers having developed the habit of ignoring the output.

I found, most unfortunately, when trying to change such error checks into obnoxiously-loud assertions that there were thousands of places in the system that were violating these preconditions and simply ignoring the errors (if there were any reported, some functions just returned without doing anything in those cases). The developers just worked around it and kept trying stuff and adding more code until their code "worked", blissfully unaware of their original mistake that they were passing nulls to a function that did not allow it.

As a result, I strongly recommend doing like this:

void MyClass::setMyString(QString input)
{
    assert(!input.isNull() && "Error: setMyString() received NULL argument.");
    m_MyString = input;
    emit myStringChanged();
}

This will bring your application to a grinding halt if the assertion fails, showing you the precise source file and line number in which the assertion failed along with this message.

This is for cases where your function is not allowed to receive null (in your case, "null" here means "empty string"). For ones that are allowed, you might simply do something else, but it wouldn't be reporting a bug (for that, use the loud and obnoxious assert which cannot be ignored).

An example of where you might use if might be like this:

bool do_something(int* out_count = 0)
{
    ...
    if (out_count)
       *out_count = ...; // write the result back.
    return true;
}

For ones that are not allowed to receive a null, use references when possible. However, you might still encounter "nulls" if you accept smart pointers, e.g., or if you expand your definition of "null" to include empty strings as another example. In those cases, assert your preconditions liberally if you can. assert has the advantage of not only bringing your application to a grinding halt (good thing), but also pointing out where it was brought to a grinding halt even outside of a debugging session (but still with a debug build, e.g.), and it's also an effective means of documentation.

qDebug is a little too silent usually. A team with good habits that never allows those outputs to be ignored might get away with keeping those to zero. But an assert will protect you even against the sloppiest teams by bringing the entire process to a grinding halt, making these programmer errors impossible to ignore. Another minor benefit of assert is that it can't slow down your release (production) builds, while if (...) qDebug(...) will.

For errors caused by external sources outside of your control (ex: failing to connect to a server that was down, failing to allocate a huge block of memory, trying to read from a file the user tried to load that was corrupt, etc), throw exceptions, but not for programmer bugs like accessing an array out of bounds or receiving a null in a place that never should receive null (unless you're working in a very mission-critical software where the software attempts to gracefully recover from even programmer bugs as opposed to making them as easy to detect and reproduce as possible). For these cases, assert.

Answer 2

As I commented, yes you should usually check against nullptr and always document if a public function accepts a null pointer or not (that it, document a function as accepting a null pointer, or as requiring a non-null valid pointer).

Checking against a nullptr is -on most current C++11 implementations- very quick: dereferencing a pointer might make a cache miss (always slow), but comparing it against 0 is a fast arithmetic operation (nearly free).

Use assert if a pointer should always be null and valid.

Use an if to handle the nullptr case if it is genuinely possible at runtime.

Use C++ references to convey that the address is always valid. That is, if you have void foo(int*) and you know that the address is never null and always valid, code a void foo(int&) function instead.

Compilers are permitted to optimize the null pointer test after you have dereferenced it:

 void foo(struct bar_st*p) {
   int x = p->somefield;
   if (!p) return; /// test can be removed by optimization

Answer 3

I figured out that you could actually pass a slot function without providing it any of its arguments, which of course would cause a Segmentation Fault if the argument was used.

What does that even mean? It's goobledygook. Generally speaking, unless you've managed to build an internally binary-incompatible project (qmake doesn't always protect from that!), Qt won't let you invoke slots with undefined arguments. If the slots have a default argument value, then calling with those arguments missing will substitute default values. But there's never a situation where there's an argument in a slot call whose value will not be set at the very least to a value-initialized default value. I.e. Qt won't blindly invoke your method/function in a binary-incompatible way. That is:

#include <QtCore>
class Class : public QObject {
   Q_OBJECT
public:
   Q_SIGNAL void mySignal1();
   Q_SIGNAL void mySignal2(const QString & = {});
   Q_SIGNAL void mySignal3(const QString &);
   Q_SIGNAL void mySignal4(QObject *);
   Q_SLOT void myMethod1(const QString & = {}) {}
   Q_SLOT void myMethod2(const QString &) {}
   Q_SLOT void myMethod3(QObject *) {}
};

int main() {
   QLoggingCategory::setFilterRules("default.warning=false");
   Class c;
   bool r;
   r = QMetaObject::invokeMethod(&c, "myMethod1");
   Q_ASSERT(r); // calls myMethod1, all parameters have values
   r = QMetaObject::invokeMethod(&c, "myMethod2");
   Q_ASSERT(!r); // doesn't call, value needed for first parameter
   r = QMetaObject::invokeMethod(&c, "myMethod2", Q_ARG(QString, "foo"));
   Q_ASSERT(r); // calls myMethod2, value is given for first parameter
   r = QMetaObject::invokeMethod(&c, "myMethod3");
   Q_ASSERT(!r); // doesn't call, value needed for first parameter
   r = QObject::connect(&c, SIGNAL(mySignal1()), &c, SLOT(myMethod1()));
   Q_ASSERT(r); // succeeds, myMethod1 has a default-value signature
   c.mySignal1(); // perfectly defined behavior: mySignal1 gets a default-constructed argument
   r = QObject::connect(&c, SIGNAL(mySignal1()), &c, SLOT(myMethod2()));
   Q_ASSERT(!r); // fails - no such slot
   r = QObject::connect(&c, SIGNAL(mySignal2()), &c, SLOT(myMethod2(QString)));
   Q_ASSERT(!r); // fails - no such signal
   r = QObject::connect(&c, SIGNAL(mySignal2(QString)), &c, SLOT(myMethod2(QString)));
   Q_ASSERT(r);
   c.mySignal2(); // mySignal2 has a default value for the argument
   r = QObject::connect(&c, SIGNAL(mySignal3(QString)), &c, SLOT(myMethod1(QString)));
   Q_ASSERT(r);
   c.mySignal3({});
   r = QObject::connect(&c, SIGNAL(mySignal4(QObject*)), &c, SLOT(myMethod3(QObject*)));
   Q_ASSERT(r);
   c.mySignal4(nullptr); // explicit call with a null object, myMethod3 is presumed to accept such
}
#include "main.moc"

In any case, using a null Qt container (i.e. a null string) is not undefined behavior (i.e. doesn't result in a segmentation fault) as long as you're not doing something thoroughly silly like not checking the length of the string you work on when the length plays a role. And it only plays a role if you're trying to use the string in a context where the length is important, e.g. if you're indexing its contents in a way that requires an index within the range of the string. Many methods don't care, i.e. QString::mid doesn't mind an invalid position - a null string is returned instead.

I am getting into the habit of checking all my arguments to make sure they are not NULL

Your example has nothing to do with checking anything against a null pointer, which is what the C NULL is!

Your example's test is fairly nonsensical unless somehow m_myString must be non-empty. I doubt that it is so. A typical Qt setter would look as follows:

void MyClass::setMyString(const QString &input) {
  if (m_MyString != input) {
    m_MyString = input;
    // do whatever is needed when the string has changed, e.g.
    // call update() if MyClass is a widget.
    emit myStringChanged();
  }
}

Note that in Qt, only the default-constructed string is null. Thus, a test for a null string is very specific: it's a test stronger than that for an empty string, and as far as other behavior goes, it doesn't make any difference: a null string and an empty string behave the same, other than the return value from isNull().

As a rule of thumb, the null-ness of a string only matters as a discriminant on output. It is a poor-man's workaround for the absence of std::optional. A null string value is/can be returned in special cases where absence of value needs to be differentiated from an empty string. As far as input goes, everything I know of in Qt treats a null string on input as an empty string, although certainly a null string (or any other null value - i.e. a null pointer or a value with isNull() == true) propagates to a null variant. But a null string does not result in an invalid variant, for example:

QVariant v;
v = "b";
Q_ASSERT(v.toString() == "b");
v = "";
Q_ASSERT(!v.isNull());
Q_ASSERT(!v.toString().isNull());
Q_ASSERT(v.toString().isEmpty());
QString b;
Q_ASSERT(b.isNull());
v = b;
Q_ASSERT(v.isNull());
Q_ASSERT(v.isValid()); // null but valid
Q_ASSERT(v.toString().isNull());
Q_ASSERT(v.toString().isEmpty());
Q_ASSERT(v.toString() == b);
Q_ASSERT(v != QVariant{}); // because it's valid

QObject o;
o.setProperty("foo", "a");
Q_ASSERT(o.property("foo") == "a");
o.setProperty("foo", QVariant{});
Q_ASSERT(!o.dynamicPropertyNames().contains("foo"));
o.setProperty("foo", QString{}); // not invalid anymore
Q_ASSERT(o.dynamicPropertyNames().contains("foo"));
Q_ASSERT(o.property("foo").isNull());

Thus, if you really for some reason don't want default-constructed (i.e. null) strings coming in, you should assert so. A debug output is insufficient, it should be at least a qWarning. But an assert would be preferred. Thus:

void MyClass::setMyString(const QString &input) {
  Q_ASSERT(!input.isNull());
  if (input.isNull()) { // for when QT_NO_DEBUG - i.e. release code
    qWarning() << "Error: MyClass::setMyString() invoked with a null parameter";
    return;
  }
  if (m_MyString != input) {
    ...
  }
}

Is this an acceptable style in all or most circumstances?

No. If it's a problem, then a mere qDebug is not enough. You need an assertion for debug builds, and at least a warning for release builds, if not a release assert. Ideally you'd implement ASSERT_ALWAYS and use it instead for such cheap checks:

#define ASSERT_ALWAYS(cond) ((cond) ? static_cast<void>(0) : qt_assert(#cond, __FILE__, __LINE__))

Should I be introducing a runtime error, halting the program, instead of simply returning the function?

It depends on the semantics - i.e. meaning of - MyString. If MyString makes absolutely no sense when empty, then you should ASSERT_ALWAYS(!input.isEmpty()) (not an isNull check) and terminate the program. The check is cheap, and the overhead of the check is nothing much compared to the cost of emitting a signal. If MyClass allows an empty MyString, then the null check is unnecessary. A null string is an empty string, but an empty string is not necessarily null.

Does this style of coding improve security?

Not really. If MyClass::MyString cannot ever be empty, then when you attempt to set it to empty, something went horribly wrong. You can't divine how benign of a problem it is, and thus ASSERT_ALWAYS is the only sane way out: log the problem and die right then and there.

If you want to fail fast like in Erlang, the asserts would throw an assert_exception of some kind, you'd write exception-safe code, and restart whatever went wrong - but such style requires discipline, and requires extra code to partition the problem into equivalents of Erlang processes that still run on one thread.

Does this style of coding improve stability?

There are only two scenarios where you are improving stability: if you do ASSERT_ALWAYS - i.e. you prevent continuation upon the first logic error, or if you write fail-fast Erlang-style code where a failing conceptual process is nothing special, and you have means of restarting it. The half-baked "check and do nothing if the argument is wrong" usually has disastrous consequences, as you're ignoring clear logic errors in the user of the class, and assuming that such errors are inconsequential. Yet the class has no knowledge of how inconsequential those errors truly are.

Are there any performance considerations, to the extent that checking for null values may take longer depending on the type?

Again, since you're talking of Qt containers here: your code elsewhere should be already coping with empty containers and not blindly assuming that they have one or more elements - presumably. If the code elsewhere can't deal with it, i.e. it's a class invariant that MyString has length 1 or more, then of course adding the check has a minuscule performance impact. The cost of the check, for all Qt containers that support null (there aren't many), is a pointer comparison, or two at most.

Beyond C++ and Qt, what other languages benefit from this style?

That's impossible to answer, since you're focused on a null value, which is an implementation detail. The null value is not important. What's important are the semantics of that value in the context it is used. If the null value violates prerequisites for a function (in this case: setMyString), then of course you should be asserting it, and because it's a cheap check - it should be an ASSERT_ALWAYS. If the coding style and/or environment supports fail-fast, then you should fail, and let the user code restart the entire failed process (here process means a contained unit of work and has nothing to do with OS processes!).

Are there any significant drawbacks worth considering?

Yes. The fact that you're looking at it from the wrong side - a null QString is almost never a problem. The semantics of your class, and of the null value in that context, and class invariants, and function prerequisites, are what's important, and you should be looking at the problem in those terms. I have a feeling that whatever was the perceived source of the segmentation faults had zilch to do with null QStrings. Those are, after all, special cases of empty strings, and you can manipulate them just as you could any other empty string. E.g. this is valid:

QString a; // null string is also an empty string
Q_ASSERT(a.isNull());
Q_ASSERT(a.isEmpty());
qDebug() << a;
Q_ASSERT(a.size() == 0);
a += QStringLiteral("b");
Q_ASSERT(a == QStringLiteral("b"));

And just to be completely clear: the following is not a null string. It is not a null string in any shape nor form, because it's not a string to begin with.

QString *s = nullptr;

This is a null pointer-to-string. NOT a STRING. Big difference. And the question contained exactly zero pointers-to-strings, mind you.