25. Do not compare 'this' to nullptr anymore

The fragment is taken from CoreCLR project. This dangerous code is detected by the following PVS-Studio 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. I 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.

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. I 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, I 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 I 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.

results matching ""

    No results matching ""