You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "jihuayu (via GitHub)" <gi...@apache.org> on 2023/05/30 11:26:35 UTC
[GitHub] [incubator-kvrocks] jihuayu opened a new pull request, #1479: Fix all weird out parameters `int *ret` in the codebase
jihuayu opened a new pull request, #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479
I have followed the rules below and completed all the modifications.
https://github.com/apache/incubator-kvrocks/issues/1451#issuecomment-1565481854
> ### 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 `bool *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`.**
>
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1569506521
Hello, I have fixed all mentioned above. Is there anything else that needs to be corrected?
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568585951
> @git-hulk I don't think using `redis::Bool` is appropriate. The meaning of `redis::Integer` function is to return a Integer RESP, not process a Integer. Even though we are passing in a `bool` here, we need to return a Integer RESP, so I think `redis::Integer` function is more appropriate.
We already have `redis::Integer` function now, what I mean is to add redis::Bool based on the Integer function like below:
```
std::string Bool(b bool) {
return redis::Integer(ret ? 1 : 0)
}
```
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568423017
@jihuayu I think instead of adding a new function we can simply write `*output = redis::Integer(ret ? 1 : 0);` where `ret` is boolean.
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568371041
> @jihuayu Please, have a look at `String::CAD` where `flag` is of type `*int` and inside a function, you assign boolean and integer values to the `flag` variable. The first issue is that it's not consistent. The second one is, again, an implicit conversion between an integer value and a boolean value.
I will fix it.
> @jihuayu In `CommandSIsMember` you changed `int` to `bool` which is good. However, the line `*output = redis::Integer(ret);` means the boolean value is implicitly converted to an integer value. Am I right?
@torwig Thank you for bringing this to my attention. While it is true that `bool` is a [integral types](https://en.cppreference.com/w/cpp/types/is_integral) that can be passed to the `Integer` function, there is no `std::to_string` function for `bool` parameters. This could potentially lead to implicit type conversion.
There are two possible solutions to the current issue:
1. Change `*output = redis::Integer(ret)` to `*output = redis::Integer(static_cast<int>(ret))` in order to prevent implicit type conversion.
2. Add a new `Integer` function to handle cases where the parameter type is `bool`.
```c++
std::string Integer(bool data) { return Integer(data ? 1 : 0); }
```
Personally, I am inclined towards the second solution. Do you have any other suggestions?
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568303680
@jihuayu In `CommandSIsMember` you changed `int` to `bool` which is good. However, the line `*output = redis::Integer(ret);` means the boolean value is implicitly converted to an integer value. Am I right?
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568445821
But now there's another issue. If someone passes a bool type arguments to the `redis::Integer` function, our clang-tidy tool won't be able to find it.
Shouldn't we modify `redis::Integer` function to prevent others from passing bool type arguments?
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568327240
@jihuayu Please, have a look at `String::CAD` where `flag` is of type `*int` and inside a function, you assign boolean and integer values to the `flag` variable. The first issue is that it's not consistent. The second one is, again, an implicit conversion between an integer value and a boolean value.
--
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 merged pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568438552
@torwig Sounds good to me too.
There are only 4 places where `ret` is `boolean`. Maybe we don't need to add an new function for these 4 calls. Keeping it simple is also a good idea.
I will change it to `*output = redis::Integer(ret ? 1 : 0);`
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568506914
@jihuayu Perhaps, we can add `readability-implicit-bool-conversion` to the `clang-tidy` list of checks with the `AllowPointerConditions` options set to `true`. However, it may be the topic of another PR if other contributors think it's a good idea because adding this check needs refactoring some code.
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568684138
> > @git-hulk I don't think using `redis::Bool` is appropriate. The meaning of `redis::Integer` function is to return a Integer RESP, not process a Integer. Even though we are passing in a `bool` here, we need to return a Integer RESP, so I think `redis::Integer` function is more appropriate.
>
> @jihuayu We already have `redis::Integer` function now, what I mean is to add redis::Bool based on the Integer function like below:
>
> ```
> std::string Bool(b bool) {
> return redis::Integer(b ? 1 : 0)
> }
> ```
I think `Bool` is not concrete here. The returned string is a integer string, like `1`, `0` rather than `true` `false`.
Current solution LGTM.
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568413637
@git-hulk
I don't think using `redis::Bool` is appropriate.
The meaning of `redis::Integer` function is to return a Integer RESP, not process a Integer.
Even though we are passing in a `bool` here, we need to return a Integer RESP, so I think `redis::Integer` function is more appropriate.
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1569359434
> > > @git-hulk I don't think using `redis::Bool` is appropriate. The meaning of `redis::Integer` function is to return a Integer RESP, not process a Integer. Even though we are passing in a `bool` here, we need to return a Integer RESP, so I think `redis::Integer` function is more appropriate.
> >
> >
> > @jihuayu We already have `redis::Integer` function now, what I mean is to add redis::Bool based on the Integer function like below:
> > ```
> > std::string Bool(b bool) {
> > return redis::Integer(b ? 1 : 0)
> > }
> > ```
>
> I think function name `Bool` is not concrete here. The returned string is an integer string, like `1`, `0` rather than `true` `false`. The current solution LGTM.
The current solution is good for me. My point is from the reply API, we indeed have the boolean type though it will convert to the integer string in the underly.
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1569514254
@jihuayu Thanks for your contribution.
--
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 pull request #1479: Fix all weird out parameters `int *ret` in the codebase
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1479:
URL: https://github.com/apache/incubator-kvrocks/pull/1479#issuecomment-1568385711
@jihuayu How about adding the `redis::Bool` function to convert the boolean value to RESP?
--
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