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

[FEATURE] nodiscard attribute on bool-returning functions (and maybe others?) #370

Open
Cyclic3 opened this issue Jun 10, 2022 · 1 comment

Comments

@Cyclic3
Copy link

Cyclic3 commented Jun 10, 2022

Is your feature request related to a problem? Please describe.
Since redis-plus-plus has gone down the return-code route of error handling, it is very easy to forget to check a return code, and therefore for failures to be ignored. This has caused countless bugs and security holes in C code, and requires careful reading through of the code to make sure that every case has been handled.

Describe the solution you'd like
Luckily, we can very easily flag up cases where developers have missed handling the return code. C++17 added the [[nodiscard]] attribute, and many platforms have their own versions predating this standard.

I'd be very happy to make a pull request for this, but I want to see if a) this is wanted, and b) if there are any further ideas people have.

Describe alternatives you've considered
Of course, well-written and coordinated programs would not require this, but I am yet to meet a developer who doesn't occasionally make mistakes like this in a rush. Adding a warning would eliminate practically all such mistakes, and in the rare cases where checking it is not required, a simple (void) cast will both silence the warning and indicate intention.

Additional context
For the sake of diplomacy, I'd like to state that I have no issue with return codes over exceptions, and would 100% agree with the choice made with this library.

@sewenew
Copy link
Owner

sewenew commented Jun 12, 2022

Since redis-plus-plus has gone down the return-code route of error handling

NO. In fact, redis-plus-plus uses exception, instead of return code, to report error. Some Redis' methods return bool. However, when it returns false, it doesn't mean that some error happens. Instead, it means Redis server returns an integer reply of 0.

Redis protocol (RESP 2) doesn't have bool reply. I converted an integer reply to a bool reply, if and only if the integer reply can either be 1 or 0. However, that seems to be a bad idea. Since Redis might change the command to return other integers (with some new options) in some future version. And there's plan to correct this behavior, i.e. revert these methods to return long long instead.

There's an exception, the SET command. If client sends it without NX or XX, it always returns "OK" status reply. Otherwise, it might return nil reply (if the specified option doesn't match). In this case, I converted "OK" status as true, and nil reply as false.

C++17 added the [[nodiscard]] attribute

I like this feature. However, I'm not sure whether it's still helpful, if all bool-returning methods have been changed to long long-returning method. Also it might not be appropriate for SET command. Since in most cases, i.e. without NX or XX option, we don't need to check the return value. [[nodiscard]] might add some burden to users.

So I think it might be better to postpone the decision until we modify bool-returning methods.

Anyway, thanks for your suggestion and sorry for the late reply!

Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants