chore(zset_family/score_map): Replace sds arguments with string_view #4738
Conversation
abhijat
changed the title
abhijat
changed the title
|
test failure seems related to this change: passing in an empty string view ends up being treated as nullptr of zero size, which is different from the behavior for sds |
reproducible by adding an empty field in unit test for zadd. |
THe usages of WrapSds are removed from zset_family calls. In most cases it is a straightforward change to use string_view or string_view.data(), string_view.size(). In one case a wrapper had to be introduced. Signed-off-by: Abhijat Malviya <[email protected]>
abhijat
force-pushed
the
abhijat/chore/cleanup_sds_in_sorted_map
branch
from
db97e4f to
24b3723
Compare
abhijat
changed the title
src/core/sorted_map.cc
Outdated
| @@ -108,7 +108,8 @@ constexpr unsigned kConvFlags = DoubleToStringConverter::UNIQUE_ZERO; | |||
| DoubleToStringConverter score_conv(kConvFlags, "inf", "nan", 'e', -6, 21, 6, 0); | |||
|
|
|||
| // Copied from redis code but uses double_conversion to encode double values. | |||
| unsigned char* zzlInsertAt(unsigned char* zl, unsigned char* eptr, sds ele, double score) { | |||
| unsigned char* zzlInsertAt(unsigned char* zl, unsigned char* eptr, const unsigned char* ele, | |||
There was a problem hiding this comment.
nit: I would use const char* ele to avoid multiple castings on the caller side.
Maybe even better: it's now in our c++ code base, so you could even keep string_view ele here and make the safe conversion to ""sv in this function.
There was a problem hiding this comment.
btw, to signify it's ours - please rename it to ZzlInsertAt
There was a problem hiding this comment.
changed name and signature
Signed-off-by: Abhijat Malviya <[email protected]>
| // results in the replace operation being applied on the listpack. In addition to being wrong, it | ||
| // also causes assertion failures. To circumvent this corner case we pass here a string view | ||
| // pointing to an empty string on the stack, which has a non-null data field. | ||
| if (ele.data() == nullptr) { |
There was a problem hiding this comment.
Why don't we patch lpAppend ? Or if we want to enforce a contract, why not simply introduce a wrapper that does this check for us here and use it. That way, we save this "hack"?
Not that it matters that much, it's a few lines
There was a problem hiding this comment.
we do not change code under redis directory and this function is the wrapper
There was a problem hiding this comment.
oh right you want to keep the diffs clean
The usages of WrapSds are removed from zset_family calls.
In most cases it is a straightforward change to use string_view or string_view.data(), string_view.size().
In one case a wrapper had to be introduced.
fixes #4639