Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Not a thorough review but just some things which accumulated over time. |
|
| << " " << ATTR_CALL_ARGVALUETYPE << "=\"" << static_cast<unsigned>(callValueType) << "\"" | ||
| << " " << ATTR_CALL_ARGVALUE << "=\"" << callArgValue.value << "\"" | ||
| << " " << ATTR_CALL_UNKNOWN_FUNCTION_RETURN << "=\"" << static_cast<int>(callArgValue.unknownFunctionReturn) << "\""; | ||
| << " " << ATTR_CALL_UNKNOWN_FUNCTION_RETURN << "=\"" << static_cast<unsigned>(callArgValue.unknownFunctionReturn) << "\""; |
There was a problem hiding this comment.
Why is it important to use unsigned instead of int.
I am not against this and would likely accept such unsigned use if this was new code. But if you refactor you need to have some important reasons. We could get a commit battle where somebody else thinks that it should be plain int and send me a PR about that. I see no clear reasons to use neither int nor unsigned here.
Using unsigned just because the value is always positive is not a good reason in my opinion. That is dangerous approach and leads to conversion problems.
There was a problem hiding this comment.
The underlying type of the enum is unsigned and we were casting it to a signed type. But it is uint8_t which will be treated like char in operator<< so we need to cast it to something not handled as a number. While int has a wide enough range even if we might move it up to a 16-bit type it would be cleaner to leave the signedness intact.
There was a problem hiding this comment.
I believe unsigned integers should be avoided.
There was a problem hiding this comment.
So numbers above 2^63-1 do not exist?
There was a problem hiding this comment.
we don't need to care about that problem here.
- if you do need such numbers then feel free to use unsigned.
- if you need to perform bitoperations that might involve the "sign" bit then please use unsigned.
but in other cases, in general I feel that "unsigned" should just be avoided. If something can be signed then let's make it signed.
I agree with the Google C++ style guide says about unsigned integers:
https://google.github.io/styleguide/cppguide.html
try not to mix signedness, and try to avoid unsigned types (except for representing bitfields or modular arithmetic). Do not use an unsigned type merely to assert that a variable is non-negative.
The enum has type uint8_t it could be int8_t instead. but as far as I see it makes no technical difference as we know the values. So I don't care a lot in this case.
3 participants
No description provided.