-
Notifications
You must be signed in to change notification settings - Fork 715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code cleanup in object.c to support future work #1659
Conversation
f3bac00
to
fd3e5e7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1659 +/- ##
=========================================
Coverage 71.02% 71.03%
=========================================
Files 121 121
Lines 65176 65233 +57
=========================================
+ Hits 46290 46336 +46
- Misses 18886 18897 +11
|
…: o to refer to robj, val/ptr/val_ptr to refer to object's value. Add static to helpers Signed-off-by: Rain Valentine <[email protected]>
fd3e5e7
to
be19659
Compare
Signed-off-by: Rain Valentine <[email protected]>
I'm combining this with #1658 I think it'll be cleaner |
robj *objectSetExpire(robj *val, long long expire); | ||
sds objectGetKey(const robj *val); | ||
long long objectGetExpire(const robj *val); | ||
robj *createObjectWithEmbeddedData(int type, void *val_ptr, const sds key, long long expire, size_t val_embedded_size, size_t *val_embed_capacity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deduplication is good. Thanks for that.
I don't think we should expose this function. It's more complex than what the caller needs. So far, embedded strings are created using createStringObject
with size checks inside of object.c to decide to embed or not. We can expose createStringObjectWithKeyAndExpire
too if you want.
For non-strings, I think we should still expose createObjectWithKeyAndExpire
and make it a thin wrapper of createObjectWithEmbeddedData
which can be internal.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point. It makes sense not to expose that complex function signature. For now I think it makes sense not to expose functions we don't actually use outside of object.c - we can always expose them later when we need them.
It kind of makes sense to have createObjectWithKeyAndExpire, but it isn't used anywhere. Do we have a near-future use for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, if it's not used, let's not expose it.
I have a local branch where I want to create the object with expire from the start in commands like SETEX, but I don't think I'll prioritize it soon.
Summary of changes:
My primary goal is to make the structure and memory layout of robj easier to change and work with in the future. This will support future work like eliminating the 8 byte
ptr
when value is embedded directly, and potentially expand value embedding beyondOBJ_ENCODING_EMBSTR
andOBJ_ENCODING_INT
.I'm a bit unsure about the function signature of
createObjectWithEmbeddedData
- let me know if you have a cleaner idea!