Rework calls to memory routines

[matt-gretton-dann]

This set of commits reworks memory routine usage. It does two things:

• Add xalloc() routines which wrap the alloc() routines and ensure that on errors we abort. This simplifies the calls to ERR_CRITICAL in a more debuggable way (we can stack trace through the abort call to find the specific failure).
• Replace xmalloc() calls with xcalloc() calls (this is an updated form of #195.

[matt-gretton-dann]

For later - do you think there is value some in sort of optional (#define TEST_MEMORY) automatic deconstruction at the end with these?

I did try this - but the sequence is complicated enough and with conditionals that I got confused and stopped with other higher priority things. Which isn't to say this is a bad idea - getting a clean bill from valgrind is useful as then any reports it does give are definitely issues.

I think the ultimate solution is to move to C++ classes,use RAII, and will free their memory automatically as the objects are destructed. This will take time though.

[ZedThree]

What's the policy on templates in the code? Lots of the calls look like

variable = (type*)Memory::xcalloc(nelems, sizeof(type));

which involves repeating the type, as well as a C-style cast. Adding something like:

template <typename T>
T* Memory::xcalloc(std::size_t nelems) noexcept {
  return static_cast<T*>(Memory::xcalloc(nelems, sizeof(T)));
}

would make the calls look like:

variable = Memory::xcalloc<type>(nelems);

and in some places can even avoid repeating the type three times by using auto:

-   char** AdunitNames, * AdunitNamesBuf;
-   if (!(AdunitNames = (char**)malloc(3 * ADUNIT_LOOKUP_SIZE * sizeof(char*)))) ERR_CRITICAL("Unable to allocate temp storage\n");
-   if (!(AdunitNamesBuf = (char*)malloc(3 * ADUNIT_LOOKUP_SIZE * 360 * sizeof(char)))) ERR_CRITICAL("Unable to allocate temp storage\n");
+   auto AdunitNames = Memory::xcalloc<char*>(3 * ADUNIT_LOOKUP_SIZE);
+   auto AdunitNamesBuf = Memory::xcalloc<char*>(3 * ADUNIT_LOOKUP_SIZE * 360);

[matt-gretton-dann]

What's the policy on templates in the code? Lots of the calls look like

variable = (type*)Memory::xcalloc(nelems, sizeof(type));

which involves repeating the type, as well as a C-style cast. Adding something like:

template <typename T>
T* Memory::xcalloc(std::size_t nelems) noexcept {
  return static_cast<T*>(Memory::xcalloc(nelems, sizeof(T)));
}

would make the calls look like:

variable = Memory::xcalloc<type>(nelems);

and in some places can even avoid repeating the type three times by using auto:

- char** AdunitNames, * AdunitNamesBuf;
- if (!(AdunitNames = (char**)malloc(3 * ADUNIT_LOOKUP_SIZE * sizeof(char*)))) ERR_CRITICAL("Unable to allocate temp storage\n");
- if (!(AdunitNamesBuf = (char*)malloc(3 * ADUNIT_LOOKUP_SIZE * 360 * sizeof(char)))) ERR_CRITICAL("Unable to allocate temp storage\n");
+ auto AdunitNames = Memory::xcalloc<char*>(3 * ADUNIT_LOOKUP_SIZE);
+ auto AdunitNamesBuf = Memory::xcalloc<char*>(3 * ADUNIT_LOOKUP_SIZE * 360);

Templates are fine. However, I take x* routines as being simple wrappers around standard library functions with error handling and implementation defined behaviours 'smoothed out'. This PR is that change.

I don't dislike your suggestions - however I wonder whether being a bit more piecemeal but radical in changes using standard library features (unique_ptr, shared_ptr and others) will be better.

[zlatanvasovic]

@matt-gretton-dann Great change! You may be interested to close #195, since this supersedes it.