You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "PragmaTwice (via GitHub)" <gi...@apache.org> on 2023/05/20 08:58:15 UTC

[GitHub] [incubator-kvrocks] PragmaTwice opened a new issue, #1451: Fix all weird out parameters `int *ret` in codebase

PragmaTwice opened a new issue, #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451

   ### Search before asking
   
   - [X] I had searched in the [issues](https://github.com/apache/incubator-kvrocks/issues) and found no similar issues.
   
   
   ### Motivation
   
   Refer to https://github.com/apache/incubator-kvrocks/pull/1444#discussion_r1199580764.
   > Maybe not related to this PR, but there are two points:
   >
   > `ret` is not a descriptive word
   > `int` type is not fit here: the return variable `members.size()` is typed `size_t`, not `int`
   
   ### Solution
   
   _No response_
   
   ### Are you willing to submit a PR?
   
   - [ ] I'm willing to submit a PR!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1565489041

   I think you can replace it with `bool` if the return value is just a boolean variable.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] jihuayu commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1562456236

   Hello, I want to try it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1563831281

   Yes, it's fine since the change is intuitive.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] infdahai commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1572376250

   @git-hulk it seems the issue should be closed?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] jihuayu commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1565506234

   > I think you can replace it with `bool` if the return variable is just a boolean value.
   
   Thank you for your suggestion. I will do that.
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1572397199

   @infdahai Yes, thanks for your reminder.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] torwig commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1562480953

   @infdahai You mean `size_t` by typing `usize`?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] jihuayu commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1565481854

   
   
   I have read through the relevant code and have decided to make modifications according to the following rules:
   
   ### The files match `types/redis_*.h`
   
   - For the function similar to [`Set#IsMember`](https://github.com/apache/incubator-kvrocks/blob/unstable/src/types/redis_set.h#L36) or [`String#SetNX`](https://github.com/apache/incubator-kvrocks/blob/unstable/src/types/redis_string.h#L47) that `ret` means the flag, I will change it to `int *flag`
   
   - For the function similar to [`Set#Remove`](https://github.com/apache/incubator-kvrocks/blob/unstable/src/types/redis_set.h#L39) that `ret` means the number of removed elements, I will change it to `uint64_t *removed_cnt`
   
   - For the function similar to [`List#Size`](https://github.com/apache/incubator-kvrocks/blob/unstable/src/types/redis_list.h#L36) that `ret` means the size of list, I will change it to `uint64_t *size`
   
   - For the function similar to [`List#Size`](https://github.com/apache/incubator-kvrocks/blob/unstable/src/types/redis_list.h#L36) that `ret` means the size of list, I will change it to `uint64_t *size`
   
   - For the function similar to [`String#IncrBy`](https://github.com/apache/incubator-kvrocks/blob/unstable/src/types/redis_string.h#L50) that `ret` means the new value, I will change it to `int64_t *new_value`
   
   - In cases where `ret` may be negative, I will set the type to `int*`.
   
   ### The files match `commands/cmd_*.cc`
   
   I will change the type of `int ret` to the correct type, but I will **not change the variable name** because it represents the return value of the Redis command.
   
   ### The test files
   
   I will change the type of `int ret` to the correct type, and I will decide whether to modify the variable name based on the situation.
   
   
   ### All files
   
   I will remove all unnecessary `static_cast` statements.
   
   **I will use `uint64_t`([`Metadata#size`](https://github.com/apache/incubator-kvrocks/blob/unstable/src/storage/redis_metadata.h#L121) type) as the type for length everywhere, instead of the `size_t`.**
   
   
   
   Do you think these are acceptable? I would appreciate hearing your thoughts and opinions.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] infdahai commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1562507287

   Typos. Thanks for pointing it out
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] PragmaTwice commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1555870365

   `Integer` can accept all [integral types](https://en.cppreference.com/w/cpp/types/is_integral), which includes `size_t`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] infdahai commented on issue #1451: Fix all weird out parameters `int *ret` in codebase

Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1555868062

   @PragmaTwice I exec the statement `*output = redis::Integer(ret);` to return the result to the client in cmd_set.cc.
   https://github.com/apache/incubator-kvrocks/blob/fb0e3d42031a6e0691c7a736711d0a01d72bdb1a/src/server/redis_reply.h#L38-L41
   
   Add the `Integer` func checks whether compatibility with `int` is required(Of course, `size_t` is right). I see src/types/*.h and many funcs with this situation use `int`. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] infdahai commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1562476706

   > Hello, I want to try it.
   
   Go ahead. It seems you will replace the `int` type with a more precise type, specially `usize`.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] jihuayu commented on issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on issue #1451:
URL: https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1563828625

   This issue involves many files. Should I submit one PR with all of the changes?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kvrocks] git-hulk closed issue #1451: Fix all weird out parameters `int *ret` in the codebase

Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk closed issue #1451: Fix all weird out parameters `int *ret` in the codebase
URL: https://github.com/apache/incubator-kvrocks/issues/1451


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org