Remove unnecessary null pointer checks

[elfring]

An extra null pointer check is not needed in functions like the following.

• NpcEdit::closeEvent
• ScriptEditorWindow::setLang
• WLD_SetPoint

[Wohlstand]

The reason why they exist is a set of crashes which I have got on some platforms. I might remove them with some exceptions (when the future logic is relying on null-ness of this pointer and no need to poke delete x; again when it's unnecessary here).

But yeah, when the logic is:

~xxx()
{
    delete x;
}

Then, I might avoid this null check here. However, when out of destructor:

if(x)
{
    delete x;
    x = nullptr;
}

I prefer to don't execute this code when no needed. I also in some cases have put extra calls related to clean-up, etc. Mainly, when these code pieces are out of destructor and the x will be reconstructed again.

[elfring]

• Which “platforms” do you know that show difficulties with the handling of null pointers for the C++ delete operator?
• Do you expect that non-null pointers will usually be used more often?

[Wohlstand]

Which “platforms” do you know that show difficulties with the handling of null pointers for the C++ delete operator?

In my memory, some crashes where happen on Windows cases with some MinGW compilers (5 and older versions), on Linux some GCC toolchains also have reproduction, idk about macOS and Haiku as I have avoided deletions of nulls in a past until I got these platforms support. However, I gonna make a test on all platforms I have (Linux, Windows, macOS, Haiku) to verify this...

Do you expect that non-null pointers will usually be used more often?

I had several bugs with that which I have fixed later, yeah. When the pointer is not null and it pointers some odd place, then, a crash will happen in any way.

[elfring]

• How do you think about to use a development tool like “clang-tidy” for corresponding source code adjustments?
• Would you like to wrap any pointers with the class template “std::unique_ptr”?

[Wohlstand]

Okay, a little unit test:

#include <stdio.h>

int main()
{
    printf("new\n");
    int *ko = new int();
    *ko = 42;
    printf("delete [0x%zX]\n", (size_t)ko);
    delete ko;
    printf("ko = 0; [0x%zX]\n", (size_t)ko);
    ko = 0;
    printf("delete again [0x%zX]\n", (size_t)ko);
    delete ko;

    printf("new\n");
    ko = new int();
    *ko = 666;
    printf("delete [0x%zX]\n", (size_t)ko);
    delete ko;
    printf("delete again [0x%zX]\n", (size_t)ko);
    delete ko;

    printf("everything is safe! [0x%zX]\n", (size_t)ko);

    return 0;
}

The first test I have done on Linux with GCC 7.4 (which is my main platform):

$ g++ delete-and-crash-me.cpp -o crashme
$ ./crashme 
new
delete [0x5643B2658280]
ko = 0; [0x5643B2658280]
delete again [0x0]
new
delete [0x5643B2658280]
delete again [0x5643B2658280]
everything is safe! [0x5643B2658280]
$ g++ -m32 delete-and-crash-me.cpp -o crashme
$ ./crashme 
new
delete [0x5738DF80]
ko = 0; [0x5738DF80]
delete again [0x0]
new
delete [0x5738DF80]
delete again [0x5738DF80]
everything is safe! [0x5738DF80]

And looks like it didn't crashed even pointer is not null (by the logic, it should crash), going to test other platforms...

How do you think about to use a development tool like “clang-tidy” for corresponding source code adjustments?

Yeah, I going to use some helpful tools to refactor my entire code, I want to clean-up the code as a whole to make it more friendly and more stable.

Would you like to wrap any pointers with the class template “std::unique_ptr”?

It's dependent on the situation: when the pointer is an alien object, it shouldn't be packed into a container like this. However, when speaking about internal heap objects, they are should use smart pointer containers.

[Wohlstand]

On Windows: The code didn't crashed with MinGW 6.3:

C:\Users\Wohlstand\.PGE_Project>C:\MinGW\bin\g++.exe delete-and-crash-me.cpp -o crashme

C:\Users\Wohlstand\.PGE_Project>crashme
new
delete [0xzX]
ko = 0; [0xzX]
delete again [0xzX]
new
delete [0xzX]
delete again [0xzX]
everything is safe! [0xzX]

C:\Users\Wohlstand\.PGE_Project>

However, it crashed when I built it with MSVC2015 (also with MSVC2017 and MSVC2019) on attempt to delete non-null pointer. Going to macOS...

[elfring]

// …
    printf("delete again [0x%zX]\n", (size_t)ko);
    delete ko;
// …

• I suggest to take another look at answers for the question “What happens in a double delete?”.
• How do you think about to avoid that such a test case depends on undefined behaviour?

[Wohlstand]

On macOS with CLang it also has crashed:

$ clang++ delete-and-crash-me.cpp -o crashme
$ ./crashme
new
delete [0x7FB907400620]
ko = 0; [0x7FB907400620]
delete again [0x0]
new
delete [0x7FB907400620]
delete again [0x7FB907400620]
crashme(954,0x10c2255c0) malloc: *** error for object 0x7fb907400620: pointer being freed was not allocated
crashme(954,0x10c2255c0) malloc: *** set a breakpoint in malloc_error_break to debug
Abort trap: 6

I suggest to take another look at answers for the question “What happens in a double delete?”.

The quite:

It causes undefined behaviour. Anything can happen. In practice, a runtime crash is probably what I'd expect.

Yeah, therefore I always nulling pointers after delete: no pointers should be non-null when an object does not exist.

How do you think about to avoid that such a test case depends on undefined behaviour?

That right, except for the cases of old and/or buggy compilers where behavior is different from the standard.

[Wohlstand]

Okay, there are no null checks here, and also, I did some refactoring here to make names be more clear and std::unique_ptr is used :fox_face: Anyway, there are still lots of other places needed for refactoring, most of code made in 2014~2015 years and it wasn't good.

[Wohlstand]

Okay, I got the REAL issue at my other project (that I do at my work as an employee). I had to leave several

delete some;
some = nullptr;

and I began to get a lot of random crashes. Once I added the check:

if(some)
    delete some;
some = nullptr;

the issue has gone. Therefore I think, null checks should be for the best compatibility against various systems and platforms. I did use various versions of Qt, gcc and clang, both giving this crashing issue when no null check for the pointer.

[elfring]

Once I added the check:

if(some)
   delete some;
some = nullptr;

the issue has gone.

I find such information strange.

• Do any other factors need further clarification for desired standard compliance?
• Can any smart pointers help a bit more here?