Do not compare ‘this’ to nullptr anymore

The fragment is taken from CoreCLR project. This dangerous code is detected by the following diagnostic: V704 ‘this == nullptr’ expression should be avoided – this expression is always false on newer compilers, because ‘this’ pointer can never be NULL.

bool FieldSeqNode::IsFirstElemFieldSeq()
{
  if (this == nullptr)
    return false;
  return m_fieldHnd == FieldSeqStore::FirstElemPseudoField;
}

Explanation

People used to compare this pointer with 0 / NULL / nullptr. It was a common situation when C++ was only in the beginning of its development. We have found such fragments doing “archaeological” research. We suggest reading about them in an article about checking Cfront. Moreover, in those days the value of this pointer could be changed, but it was so long ago that it was forgotten.

Let’s go back to the comparison of this with nullptr.

Now it is illegal. According to modern C++ standards, this can NEVER be equal to nullptr.

16i6p8

Formally the call of the IsFirstElemFieldSeq() method for a null-pointer this according to C++ standard leads to undefined behavior.

It seems that if this==0, then there is no access to the fields of this class while the method is executed. But in reality there are two possible unfavorable ways of such code implementation. According to C++ standards, this pointer can never be null, so the compiler can optimize the method call, by simplifying it to:

bool FieldSeqNode::IsFirstElemFieldSeq()
{
  return m_fieldHnd == FieldSeqStore::FirstElemPseudoField;
}

There is one more pitfall, by the way. Suppose there is the following inheritance hierarchy.

class X: public Y, public FieldSeqNode { .... };
....
X * nullX = NULL;
X->IsFirstElemFieldSeq();

Suppose that the Y class size is 8 bytes. Then the source pointer NULL (0x00000000) will be corrected in such a way, so that it points to the beginning of FieldSeqNode sub object. Then you have to offset it to sizeof(Y) byte. So this in the IsFirstElemFieldSeq() function will be 0x00000008. The “this == 0” check has completely lost its sense.

Correct code

It’s really hard to give an example of correct code. It won’t be enough to just remove this condition from the function. You have to do the code refactoring in such a way that you will never call the function, using the null pointer.

Recommendation

So, now the “if (this == nullptr)” is outlawed. However, you can see this code in many applications and libraries quite often (MFC library for instance). That’s why Visual C++ is still diligently comparing this to 0. We guess the compiler developers are not so crazy as to remove code that has been working properly for a dozen years.

But the law was enacted. So for a start let’s avoid comparing this to null. And once you have some free time, it will be really useful to check out all the illegal comparisons, and rewrite the code.

Most likely the compilers will act in the following way. First they will give us comparison warnings. Perhaps they are already giving them, we haven’t studied this question. And then at some point they’ll fully support the new standard, and your code will cease working altogether. So we strongly recommend that you start obeying the law, it will be helpful later on.

P.S. When refactoring you may need the Null object pattern.

Additional links on the topic:

  1. Still Comparing “this” Pointer to Null?
  2. Diagnostic V704.

Written by Andrey Karpov.
This error was found with PVS-Studio static analysis tool.

One thought on “Do not compare ‘this’ to nullptr anymore

  1. And of course the ban is badly thought out and stupid.

    This can be null, in fact it often is.

    We now live in a multi-threaded world and making sure that things get torn down in the correct order can actually become impossible. Add in the continual quest for 100% cpu usage, and it becomes obvious that we have moved into the gray and horrible real world.

    In this real world a reference counted object can become null. You should of course check for null before calling a method on the pointer, but we are in the real world here. In the real world coders write code when they are tired/ short of caffeine/ under pressure/ asleep/ drunk. and mistakes can and do happen.

    A language should give you every chance to prevent bad coding crashing applications, not just say “It cannot happen, so fix your code”.

    In short saying this can never be null is a straight up lie, so a mechanism of null checking is required.

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s