Ensure nullptr checks come before dereferences

[ZedThree]

Moves some explicit nullptr checking before the first time we try to dereference that pointer. I don't think any of these can cause any problems, but it fixes some issues in the static analyses.

[dschwoerer]

Mostly minor changes.

[dschwoerer]

Fix text to not say warning in an error?

[dschwoerer]

Why is this removed?
Does the passed in options not shadow the this->options?
In that case, we pass a copy of the pointer to mesh, so that copy here would still be a nullptr?

I suggest changing options to opt, as it is done in Mesh.

[ZedThree]

It's just been moved into Mesh::Mesh where options is first used.

Note that this is the constructor, and this->options and options point to the same object.

[dschwoerer]

I disagree:

#include <cstdio>

class A{
public:
  int i;
  A(int i=1): i(i){
    i=2;
    printf("%d\n",this->i); // prints 1 - as i is a local variable
  }
};

int main(){
  A a;
  return 0;
}

As in the example above, options is the variable that was passed in.
The member variable is update in the Mesh constructor - but the local variable that has been passed in, is not. I saw the move to Mesh.

This introduces a bug - please fix it by not shadowing the member options.

I wasn't sure whether constructors would be different then other functions - that is why i wrote the above test. Constructors are not different. The only part where no shadowing is taking place is in the : i(i) part

[ZedThree]

options is a pointer, so while there is shadowing going on, it doesn't matter in this case, because options and this->options point to the same object. In your example, i is passed in by value, which is why it's different to this->i. If you pass it in by address, you'll see that i and this->i always have the same value.

Moving the null check has also not changed anything regarding the shadowing -- it was still the case before hand. To be consistent with how the rest of the options are set, we may want to move the uses of options out of the ctor into BoutMesh::load, where everything else is set. That is a separate issue from these fixes though, so I don't think it's appropriate to fix them here.

[dschwoerer]

Ok, so it was broken before, and is still broken now.
Nevertheless it is broken, and should be fixed.

But back to to the issue at hand. We are changing the value of the pointer itself, not what it is pointing to. So no - options points into the void, while this->options points to something useful.

options it self is still passed by value - it is nether a reference to a pointer nor a pointer to a pointer.
options is the value of the pointer. It might point to a real object - but that is not what we are discussing, we are talking about the value of the pointer.

It does not matter on whether it is a pointer or not:

#include <cstdio>
class A{
public:
  int * i;
  A(int * i=(int*)1): i(i){
    i=(int*)2;
    printf("%p\n",this->i);
  }
};
int main(){
  A a;
  return 0;
}

Except now it prints 0x1 rather than 1

To verify the bug compile:

include <bout.hxx>
#include "../../../src/mesh/impls/bout/boutmesh.hxx"

int main(int argc, char **argv) {

  // Initialise BOUT++, setting up mesh                                                                       
  BoutInitialise(argc, argv);

  BoutMesh fu((GridDataSource*)1,nullptr);

  BoutFinalise();

  return 0;
}

and run in gdb.
You will notice it fails as it tries to dereference the 0 - as the options are not updated.

In an fixed branch - it fails trying to delete the GridDataSource (as I was to lazy to create a real one)

[dschwoerer]

Again: throw implies an error.

[dschwoerer]

WARNING-> Error