parse functions interface

[mfatemipour]

Hi, i think it's better for parse methods to accept const redisReply& instead of redisReply&

[sewenew]

Hi @mfs-git

YES, the current implementation of parse doesn't modify redisReply object, and it can be const. Since when we parse redisReply to std::string, we copy the underlying dynamically allocated string buffer of redisReply object to create a new std::string object.

std::string parse(ParseTag<std::string>, redisReply &reply) {
    if (!reply::is_string(reply) && !reply::is_status(reply)) {
        throw ProtoError("Expect STRING reply");
    }

    if (reply.str == nullptr) {
        throw ProtoError("A null string reply");
    }

    // Old version hiredis' *redisReply::len* is of type int.
    // So we CANNOT have something like: *return {reply.str, reply.len}*.
    return std::string(reply.str, reply.len);            // <---------- string buffer is copied
}

I made it non-const, because I was trying to steal the underlying string buffer of redisReply object to create a std::string object. So that we can avoid this copy. However, so far, it seems there's no way to do this. I'm wondering if we can find a solution to steal the string buffer in the future, so the parse API was left to accept a non-const reference.

Does this non-const problem get you into any trouble? Or you just think this is not a good coding-style?

Regards

[mfatemipour]

Thanks for your quick reply :)
Actually this unnecessary non-const-ness will propagate from this library into application code (caller), and if I decide to use this library into our companies projects I should change some interfaces.

I think we should use const reply for all pars methods (this will be backward compatible) and add an overloaded std::string parse(ParseTag<std::string>, redisReply &reply) for the case you mentioned.

[sewenew]

Hi @mfs-git

Actually this unnecessary non-const-ness will propagate from this library into application code

Can you show me some code example, why this parse function will be propagate to your application code? Do you need all Redis method to be const?

I think we should use const reply for all pars methods (this will be backward compatible) and add an overloaded std::string parse(ParseTagstd::string, redisReply &reply) for the case you mentioned.

Yes, it works now. However, if we can steal the underlying string buffer in the future, and call the non-const version, I'm not sure if it will break legacy code, and I still need to take a deep look into it.

Regards