Fix all weird out parameters int *ret in the codebase

[jihuayu]

I have followed the rules below and completed all the modifications.

#1451 (comment)

The files match types/redis_*.h

• For the function similar to Set#IsMember or String#SetNX that ret means the flag, I will change it to bool *flag

• For the function similar to Set#Remove that ret means the number of removed elements, I will change it to uint64_t *removed_cnt

• For the function similar to List#Size that ret means the size of list, I will change it to uint64_t *size

• For the function similar to List#Size that ret means the size of list, I will change it to uint64_t *size

• For the function similar to String#IncrBy 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 type) as the type for length everywhere, instead of the size_t.

[torwig]

@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]

@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.

[jihuayu]

@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 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.

std::string Integer(bool data) { return Integer(data ? 1 : 0); }

Personally, I am inclined towards the second solution. Do you have any other suggestions?

[git-hulk]

@jihuayu How about adding the redis::Bool function to convert the boolean value to RESP?

[jihuayu]

@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.

[torwig]

@jihuayu I think instead of adding a new function we can simply write *output = redis::Integer(ret ? 1 : 0); where ret is boolean.

[jihuayu]

@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);

[jihuayu]

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?

[torwig]

@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.

[torwig]

Generally, LGTM.
Good job, @jihuayu !

[git-hulk]

@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)
}

[PragmaTwice]

@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.

[git-hulk]

@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.

[jihuayu]

Hello, I have fixed all mentioned above. Is there anything else that needs to be corrected?

[git-hulk]

@jihuayu Thanks for your contribution.