Skip to content
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 const_sds for const-content sds #1553

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

zuiderkwast
Copy link
Contributor

sds is a typedef of char *.

const sds means char * const, i.e. a const-pointer to non-const content.

More often, you would want const char *, i.e. a pointer to const-content. Until now, it's not possible to express that. This PR adds const_sds which is a pointer to const-content sds.

To get a const-pointer to const-content sds, you can use const const_sds.

In this PR, some uses of const sds are replaced by const_sds. We can use it more later.

Fixes #1542

@zuiderkwast zuiderkwast marked this pull request as draft January 13, 2025 11:38
Signed-off-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Jan 13, 2025
@zuiderkwast zuiderkwast marked this pull request as ready for review January 13, 2025 11:43
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.94%. Comparing base (d13aad4) to head (2ba610a).
Report is 3 commits behind head on unstable.

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #1553   +/-   ##
=========================================
  Coverage     70.94%   70.94%           
=========================================
  Files           120      120           
  Lines         65044    65046    +2     
=========================================
+ Hits          46143    46147    +4     
+ Misses        18901    18899    -2     
Files with missing lines Coverage Δ
src/sds.c 86.85% <100.00%> (+0.16%) ⬆️
src/sds.h 79.03% <100.00%> (+1.61%) ⬆️
src/server.c 87.64% <100.00%> (+0.03%) ⬆️

... and 11 files with indirect coverage changes

Signed-off-by: Viktor Söderqvist <[email protected]>
Copy link
Contributor

@rjd15372 rjd15372 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@SoftlyRaining SoftlyRaining left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! Looks good to me as-is, but any thoughts on proactively spreading const_sds to other areas of code?

@zuiderkwast
Copy link
Contributor Author

any thoughts on proactively spreading const_sds to other areas of code

When we have it, it's possible to use it in new code, so we can suggest it in code reviews.

Constness has to be added bottom-up, i.e. start in sds.c with functions like sdslen. A non-const value can always be used as a const value, but not vice versa.

We could add const in many other situations too, not related to sds. I don't think it's worth doing a lot of refactoring though just for that.

Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM even though I still think that the const should have been applied on the pointer as well like I commented in #1542
IMO we should not allow "assignment" to a const_sds.

We can always evolve to doing this as well I guess

@zuiderkwast zuiderkwast merged commit 2a1a65b into valkey-io:unstable Jan 14, 2025
58 of 59 checks passed
@zuiderkwast zuiderkwast deleted the const_sds branch January 14, 2025 09:38
proost pushed a commit to proost/valkey that referenced this pull request Jan 17, 2025
`sds` is a typedef of `char *`.

`const sds` means `char * const`, i.e. a const-pointer to non-const
content.

More often, you would want `const char *`, i.e. a pointer to
const-content. Until now, it's not possible to express that. This PR
adds `const_sds` which is a pointer to const-content sds.

To get a const-pointer to const-content sds, you can use `const
const_sds`.

In this PR, some uses of `const sds` are replaced by `const_sds`. We can
use it more later.

Fixes valkey-io#1542

---------

Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: proost <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce const Where Applicable
4 participants