Use const char pointer in redismodule.h as far as possible#10064
Use const char pointer in redismodule.h as far as possible#10064oranagra merged 3 commits intoredis:unstablefrom
Conversation
|
@oranagra I think at the API level const correctness is important, so I think we should change that. Also, it doesn't break the ABI, just a compilation warning/error. |
|
@yossigo i agree, but my concern is weather or not this is worth the breakage. but, maybe we should use this opportunity to add const correctness to all APIs? |
|
@oranagra It's hard to tell without seeing the scope of change, but I tend to say if it's not a big issue if it's only a compilation issue. I suppose most modules ship their own |
|
@ShooterIT do you wanna look into it and see the cope of doing it for the rest of the API? |
ShooterIT
changed the title
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
ok. so the only changes that where required are in Digest, Info injection, and Cluster bus messages?
i suppose all of these are relatively unused by most of the modules.
and it's interesting to see that many of the test modules didn't have to be changed.
@yossigo i'm ok with this.
ShooterIT
changed the title
|
@redis/core-team not sure if we consider that a breaking change or a major decision. |
madolson
left a comment
madolson
left a comment
There was a problem hiding this comment.
Seems reasonable to me.
6 participants
When I used C++ to develop a redis module. i used
string.data()as the second parametereleof
RedisModule_DigestAddStringBuffer, but there is a warning, since we never change theele,i think we should use
const charfor it.This PR adds const to just a handful of module APIs that required it, all not very widely used.
The implication is a breaking change in terms of compilation error that's easy to resolve, and no ABI impact.
The affected APIs are around Digest, Info injection, and Cluster bus messages.