Modernize memory management in RxD.

[1uc]

Looking at grids.{h,c}pp we find lots of code:

nrn/src/nrnpython/grids.h

Lines 99 to 107 in f74ec59

	class Grid_node {
	public:
	Grid_node* next;
	double* states; // Array of doubles representing Grid space
	double* states_x;
	double* states_y;
	double* states_z; // TODO: This is only used by ICS, is it necessary?
	double* states_cur;

nrn/src/nrnpython/grids.h

Lines 119 to 123 in f74ec59

	BoundaryConditions* bc;
	Hybrid_data* hybrid_data;
	Concentration_Pair* concentration_list;
	Current_Triple* current_list;
	ssize_t num_concentrations, num_currents;

and allocation in a ctor:

nrn/src/nrnpython/grids.cpp

Lines 254 to 257 in f74ec59

	states_x = (double*) malloc(sizeof(double) * _num_nodes);
	states_y = (double*) malloc(sizeof(double) * _num_nodes);
	states_z = (double*) malloc(sizeof(double) * _num_nodes);
	states_cur = (double*) malloc(sizeof(double) * _num_nodes);

or in random functions:

nrn/src/nrnpython/grids.cpp

Lines 836 to 837 in f74ec59

	free(ecs_tasks);
	ecs_tasks = (ECSAdiGridData*) malloc(n * sizeof(ECSAdiGridData));

with deallocation redundantly scattered across multiple dtors:

nrn/src/nrnpython/grids.cpp

Lines 1244 to 1248 in f74ec59

	ECS_Grid_node::~ECS_Grid_node() {
	int i;
	free(states_x);
	free(states_y);
	free(states_cur);

nrn/src/nrnpython/grids.cpp

Lines 1739 to 1744 in f74ec59

	ICS_Grid_node::~ICS_Grid_node() {
	int i;
	free(states_x);
	free(states_y);
	free(states_z);
	free(states_cur);

Note, that state_z is only deallocated in one of the two, creating additional complexity. (There's a comment claiming this is intended.)

There are multiple issues here:

1. malloc allocates N bytes of memory. Often it'll be zeroed out; but there's no guarantee. What it doesn't do is initialize the values correctly. This matters for non-trivial types, such as an std::vector. Non-trivial types often allocate memory; and to avoid leaking memory they also deallocate memory. If the pointer is initially put into a random state this will lead to either dereferencing that random address; or attempting to deallocate it.
2. A raw pointer doesn't carry much information compared to more modern concepts. A pointer could be any of these four things: a (non-) owning pointer to a single (multiple) element(s). Non-owning requires not deallocating, while owning requires deallocating. Single elements are deallocated (delete) differently than multiple elements (delete[]).
3. Serious risk of double free or leaks.

[ramcdougal]

Broadly, I support this, although the malloc'd stuff by definition is not suppose to initialize anything; those values are calculated later. It would be a logic error if initialization mattered. Those are all literally used as arrays of doubles and never treated as a std::vector or anything remotely like that.

[1uc]

For double* and other trivial types the argument that the allocated memory doesn't need to be initialized holds. This argument doesn't hold for non-trivial types; even double** is risky. The issue is that non-trivial types typically manage a resource, e.g. memory. When copying or moving a value, the existing resource might need to be released. This causes a problem in code such as:

auto ptr = (Current_Triple*) malloc(...);
ptr[0] = Current_Tripple(...);

because ptr[0] is uninitilized its contents are "random"/unspecified. The result is that when it's being assigned, it'll contain a size_t* to the stable identifier, this pointer is in a random state. Currently, this doesn't matter, we simply copy the pointer. Done.

However, if we wanted to change the implementation of non_owning_identifier_wihtout_container then suddenly a random pointer might be an issue. For example the reference counted non-owning identifier is expected to dereference it's pointer to the stable identifier, check its reference count and then potentially deallocate the stable identifier (for now std::shared_ptr does this for us). If the pointer to the stable identifier is random, we will first dereference, hopefully, invalid memory (and segfault); and then deallocate memory we shouldn't (causing more breakage).

The way non-trivial classes work, is that they need to be initialized:

Current_Tripple ct;
auto ptr = new Current_Tripple[n]

both ensure that ct and each element of ptr are default initialized. This implies that the pointer to the stable identifier is a nullptr; and therefore nothing bad happens.

Note that this is transient. If a class contains a member which is non-trivial, then the struct itself will be non-trivial.

Summary: I would strongly advocate to new and delete anything that isn't one of the builtin scalar types, double and int. Builtin trivial types can be modernized separately and for other reasons.