Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce shared query buffer for client reads #258
Introduce shared query buffer for client reads #258
Changes from 8 commits
5617098
44db6e2
5592840
dd182c5
0b34557
41b7c79
2041fc3
7f7f618
cdb8023
1ac1390
05224a5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't know which pattern is more established but
sdsfree
anddismissMemory
work with NULL ptrs. @madolson should we align withsdsfree
instead?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.
I don't know how pervasive it was being adopted, but the preference was that all free functions should accept NULL pointers. So I guess we can be consistent:
I know there was mentions in the past that if the compiler doesn't end up inlining this code, it will unnecessary evict memory from the instruction cache since it will load the first instruction + a cache line of some random place in memory.
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.
dismissSds
callssdsAllocPtr
which doesn't handle NULL ptrs.We can modify
dismissSds
to handle NULL. but in this case it would be reasonable to modify all thedismiss<Type>
function family. Perhaps this should be done as a dedicated PR?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.
If you really want it in a separate PR, I'm okay with that.