Should I outsource allocation algorithm? (RAII)

Question

Right now my class has a constructor, copy constructor and copy assignment operator which all do the same thing at first (allocating memory). The destructor is deallocating the memory.

class Register
{
public:
    Register()
    {
        _trampoline_address = reinterpret_cast<BYTE*>(VirtualAlloc(nullptr, _trampoline_size, MEM_COMMIT | MEM_RESERVE, PAGE_EXECUTE_READWRITE));
        if (_trampoline_address == nullptr)
        {
            throw my_exception("Could not allocate memory for trampoline function.");
        }

        //....
    }
    ~Register()
    {
        if (_trampoline_address != nullptr)
        {
            debug(VirtualFree(_trampoline_address, 0, MEM_RELEASE));
        }
    }
    Register(const Register& other)
    {
        _trampoline_address = reinterpret_cast<BYTE*>(VirtualAlloc(nullptr, _trampoline_size, MEM_COMMIT | MEM_RESERVE, PAGE_EXECUTE_READWRITE));
        if (_trampoline_address == nullptr)
        {
            throw my_exception("Could not allocate memory for trampoline function.");
        }

        //...
    }
    Register& operator= (const Register& other)
    {
        _trampoline_address = reinterpret_cast<BYTE*>(VirtualAlloc(nullptr, _trampoline_size, MEM_COMMIT | MEM_RESERVE, PAGE_EXECUTE_READWRITE));
        if (_trampoline_address == nullptr)
        {
            throw my_exception("Could not allocate memory for trampoline function.");
        }

        //....
    }

private:
    BYTE* _trampoline_address;
    static const int _trampoline_size = 20;
};

I thought about outsourcing the allocation algorithm because I use it 3 times, but I don't want that other instances of the same class type can access that function.

So what would be the proper solution to allocate memory in 3 functions in a RAII class?

Answer 1

You could create a private static function to help with the memory allocation:

static BYTE* allocate()
{
    BTYE* ptr = reinterpret_cast<BYTE*>(VirtualAlloc(nullptr, _trampoline_size, MEM_COMMIT | MEM_RESERVE, PAGE_EXECUTE_READWRITE));
    if (ptr == nullptr)
    {
        throw my_exception("Could not allocate memory for trampoline function.");
    }
    return ptr;
}

Then, the constructors can be simplified to:

Register() : _trampoline_address(allocate()) { }

Register(const Register& other) : _trampoline_address(allocate()) { }

However, the copy assignment operator needs a little bit more work. First of all, it's not clear from your posted code, what are the semantics of the assignment operator. Are you supposed to copy the data from the RHS to the LHS? If not, what role does the RHS play in the assignment operation? What are you supposed to do with the memory owned by the LHS?

Register& operator= (const Register& other)
{
   // Prevent messing with memory when dealing with self-assignment.
   if ( this != &other )
   {
      // Deal with the semantics of the operation.
   }
   return *this;
}

Answer 2

You can do one of two things:

1. Create a private function, then call it in each constructor. This still allows other member functions of the class to call the function after construction, but that should be OK (just don't call it!). Subclasses cannot call a class's private members, so you should be fine.
2. Use delegated constructors. Perform the allocation in one constructor, then call that constructor from the others. Obviously, nobody can call a constructor explicitly, so your concern should be taken care of. Possible caveat: you will need to use a C++11 compliant compiler.

Answer 3

struct Register {
  Register():
    _trampoline_address(allocate())
  {}
  Register(Register const& o):
    Register() // forward to default ctor
  {
    copy_data_from(o);
  }
  ~Register() {
    if (_trampoline_address)
      debug(VirtualFree(_trampoline_address, 0, MEM_RELEASE));
  }
  Register& operator= (const Register& o) {
    if (this != std::addressof(o))
      copy_data_from(o);
    return *this;
  }
private:
  void copy_data_from(Register const& o) {
    Assert(_tranpoline_address);
    // ...
  }
  static BYTE* allocate() {
    return reinterpret_cast<BYTE*>(
      VirtualAlloc(nullptr, _trampoline_size, MEM_COMMIT | MEM_RESERVE, PAGE_EXECUTE_READWRITE)
    );
  }
  BYTE* _trampoline_address;
  static const/*expr*/ int _trampoline_size = 20;
};

there is only one call to allocate, but I still put it in a static private method because it is messy.

I also wrote copy_data_from, because it will be used twice (once in copy-ctor, once in assignment).

I personally would be tempted to have Register() leave you with an empty buffer, only filled when used. Read functions would have to check for nullptr and allocate() if it was missing: but they have to check for nullptr anyhow. The result is a more efficient move-ctor and move-assign, creating empty arrays (and populating later) is far more efficient, etc.

In this case, allocate() turns out to be more useful. You can even have ensure_allocated(), which initializes _tranpoline_address if nullptr.