Writing a safe wrapper class for memory management of a user-defined object's pointer

Question

I'm using redis c++ client to develop client APIs which will do CRUD operations on the redis cluster. The client library that I'm using returns a pointer of a redisReply structure whenever a command is executed. Later, I'm expected to use the freeReplyObject() function on the pointer to free the memory.

In order to develop a safer way, so that I inadvertently don't use freeReplyObject() on an invalid pointer, I'm thinking about writing a wrapper class:

class reply_wrapper
{
        public:
                redisReply* p_reply;
                reply_wrapper(redisReply* reply = NULL)
                {
                        p_reply=reply;
                }

                ~reply_wrapper()
                {
                        freeReplyObject(p_reply);
                }
};

I'm thinking about constructing an object of this class, whenever I will execute a command on the server. I think by doing this I no longer will have to free the memory manually. Is this approach correct here and is there a better approach?

Please note that freeReplyObject() handles the case of null pointer.

I'm getting almost all the suggestions about using shared-pointer/unique-pointer. While I check the examples available online and see how it fits my scenario(custom destructor), I would also like to know if there's anything fundamentally wrong in my method of handling the memory above.

Did you think about `unique_ptr` with `freeReplyObject` as custom destructor? — mch, Apr 25 '19 at 08:12
@mch I don't know much about unique_ptr and how to define a custom destructor..I'll take a look. — Vishal Sharma, Apr 25 '19 at 08:14
Prefer constructor's initialiser list (not to be confused with `std::initializer_list`); with complex objects, you get direct initialisation while otherwise default-initialisation + assignment; some types (non-default-constructible ones, references) *only* can be initialised this way. (`reply_wrapper(redisReply* reply) : p_reply(reply) { }`). — Aconcagua, Apr 25 '19 at 08:23
As using C++11, you should prefer C++ *keywords* (`nullptr`) over old (obsolete?) C *macros* (`NULL`). — Aconcagua, Apr 25 '19 at 08:25
@VishalSharma _" I don't know much about unique_ptr"_ [Here's](https://en.cppreference.com/w/cpp/memory) all you need to know. — πάντα ῥεῖ, Apr 25 '19 at 08:26
In the code as is: Does `freeReplyObject` accept null pointers at all (C++ `delete` does, C's `free` function: UB!)? If not, you should add an appropriate check. — Aconcagua, Apr 25 '19 at 08:27
@Aconcagua " if (r == NULL) return;" is in the beginning of freeReplyObject() definition.(r is the pointer here of redisReply type). — Vishal Sharma, Apr 25 '19 at 08:29
@Aconcagua in C, `free(NULL)` is well-defined although some buggy runtime would not accept it (and crash). See https://stackoverflow.com/a/1938758/5470596 — YSC, Apr 25 '19 at 08:46

score 2 · Accepted Answer · answered Apr 25 '19 at 13:33

As suggested, I've used std::unique_ptr and while constructing it I had to pass a functor which calls the freeReplyObject function in it.

struct redisReplyDeleterFunctor {
      void operator()(redisReply* p) {
          freeReplyObject(p);
      }
  };

unique_ptr<redisReply, redisReplyDeleterFunctor> reply((redisReply*)(redisClusterCommand(getDbContext(),  command.c_str())));

Now I don't need to call the freeReplyObject() manually and the memory will be automatically released as soon as my unique_ptr goes out of scope.

BTW, no need to define `redisReplyDeleterFunctor`. You could use `freeReplyObject` directly as: `unique_ptr reply(object, freeReplyObject);`. Also, you could use `decltype(freeReplyObject)` instead of `void (*)(redisReply*)`. — Nawaz, Sep 13 '19 at 16:28

Writing a safe wrapper class for memory management of a user-defined object's pointer

1 Answers1