[vim/vim] Refactor/if cscope bool (PR #17978)
Damien Lejay
It has been three weeks since the first introduction of booleans in the codebase. No complaints so far. So I go forward and refactor a whole optional file.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/17978
Commit Summary
- d71ba74 refactor: use bool, true, false in if_cscope.c
- 3e3aa8b Merge branch 'master' into refactor/if_cscope-bool
File Changes
(2 files)
- M src/if_cscope.c (144)
- M src/proto/if_cscope.pro (2)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In src/if_cscope.c:
> @@ -315,46 +315,46 @@ ex_cstag(exarg_T *eap)
case 0 :
if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
Avoid writing conditions like if (ret == false).
ret is already a boolean, so you can simply write if (!ret) instead.
It's shorter, more idiomatic, and easier to read.
- if (ret == false) + if (!ret)
In src/if_cscope.c:
> }
break;
case 1 :
if (cs_check_for_tags())
{
- ret = do_tag(eap->arg, DT_JUMP, 0, eap->forceit, FALSE);
- if (ret == FALSE)
+ ret = do_tag(eap->arg, DT_JUMP, 0, eap->forceit, false);
+ if (ret == false)
Ditto.
In src/if_cscope.c:
> {
if (msg_col)
msg_putchar('\n');
if (cs_check_for_connections())
{
ret = cs_find_common("g", (char *)(eap->arg), eap->forceit,
- FALSE, FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ false, false, *eap->cmdlinep);
+ if (ret == false)
Ditto.
In src/if_cscope.c:
> cs_free_tags();
}
}
}
else if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
Ditto.
In src/if_cscope.c:
> */
static int
cs_find(exarg_T *eap)
{
char *opt, *pat;
int i;
- if (cs_check_for_connections() == FALSE)
+ if (cs_check_for_connections() == false)
Ditto.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Damien Lejay
@dlejay commented on this pull request.
In src/if_cscope.c:
> @@ -315,46 +315,46 @@ ex_cstag(exarg_T *eap)
case 0 :
if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
Thanks for your feedback.
I think it would be better to keep this PR about migrating only from TRUE/FALSE to true/false and have a separate PR for style.
Besides, I personally find if (ret == false) more legible than if (!ret) ¯\(ツ)/¯
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In src/if_cscope.c:
> @@ -315,46 +315,46 @@ ex_cstag(exarg_T *eap)
case 0 :
if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
Besides, I personally find
if (ret == false)more legible than `if (!ret)
So why did you convert it to bool type? It's meaningless unless you eliminate that (comparison with true and false).
@chrisbra
That aside, I think it would be better to convert TRUE and FALSE to true and false all at once for the entire project.
After that, it would make sense to use clang-tidy to find places where true and false are assigned to int and convert them from int to bool.
Example .clang-tidy config (untested):
Checks: > -*, readability-implicit-bool-conversion CheckOptions: - key: readability-implicit-bool-conversion.AllowIntegerConditions value: 'false' - key: readability-implicit-bool-conversion.WarnOnIntegerAssignment value: 'true' - key: readability-implicit-bool-conversion.WarnOnBooleanToIntegerAssignment value: 'true' WarningsAsErrors: '*'
Example execution (untested):
$ clang-tidy test.c -- -std=c99
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In src/if_cscope.c:
> @@ -315,46 +315,46 @@ ex_cstag(exarg_T *eap)
case 0 :
if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
It also seems possible to detect if (x == true/false).
(untested)
Checks: > -*, readability-implicit-bool-conversion, readability-simplify-boolean-expr CheckOptions: # 1. Disallow assigning bool literals (true/false) to int
- key: readability-implicit-bool-conversion.AllowIntegerConditions value: 'false' - key: readability-implicit-bool-conversion.WarnOnIntegerAssignment value: 'true' - key: readability-implicit-bool-conversion.WarnOnBooleanToIntegerAssignment value: 'true'
# 2. Disallow if (x == true/false) - key: readability-simplify-boolean-expr.ChainedConditionalReturn value: 'true' - key: readability-simplify-boolean-expr.SimplifyConditionals value: 'true' WarningsAsErrors: '*'
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Damien Lejay
@dlejay commented on this pull request.
In src/if_cscope.c:
> @@ -315,46 +315,46 @@ ex_cstag(exarg_T *eap)
case 0 :
if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
That aside, I think it would be better to convert TRUE and FALSE to true and false all at once for the entire project.
The main benefit is not about TRUE/FALSE but rather about using bool instead of int when possible. And this cannot be automated, as some functions that are currently using TRUE/FALSE still need to have int as return type. One example in this file is cs_find.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In src/if_cscope.c:
> @@ -315,46 +315,46 @@ ex_cstag(exarg_T *eap)
case 0 :
if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
First of all, I didn't say "automate."
My suggestions are:
- Replace TRUE/FALSE with true/false all at once (with one PR).
- Use
clang_tidyto find whereintshould be changed tobooland make the change manually (with multiple PRs).
...still need to have int as return type. One example in this file is cs_find.
??. Why can't it be bool?
The return type of cs_find() could be bool.
We can change the return type of the member func of cscmd_T to bool. (The return value of func is not referenced in the first place.)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#17978)Please don't do the replacement all at once, but let's go via file, otherwise I won't be able to review. I also prefer if (!false) for readability reasons.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Damien Lejay
@dlejay pushed 1 commit.
- e6af1db Merge branch 'master'
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Damien Lejay
@dlejay commented on this pull request.
In src/if_cscope.c:
> @@ -315,46 +315,46 @@ ex_cstag(exarg_T *eap)
case 0 :
if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
The return value of func cannot be changed to bool, for example cs_add returns CSCOPE_SUCCESS or CSCOPE_FAILURE, that’s either 0 or -1.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
@h-east requested changes on this pull request.
In src/if_cscope.c:
> @@ -315,46 +315,46 @@ ex_cstag(exarg_T *eap)
case 0 :
if (cs_check_for_connections())
{
- ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, FALSE,
- FALSE, *eap->cmdlinep);
- if (ret == FALSE)
+ ret = cs_find_common("g", (char *)(eap->arg), eap->forceit, false,
+ false, *eap->cmdlinep);
+ if (ret == false)
If you do not address the issues, the merge will not take place.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Damien Lejay
dlejay left a comment (vim/vim#17978)Avoid writing conditions like if (ret == false).
ret is already a boolean, so you can simply write if (!ret) instead.
Thinking again about it, I know understand what you mean. And I agree it is kind of useless to have a == with a boolean. I’ll take care of it. 👌
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Damien Lejay
Closed #17978.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Damien Lejay
dlejay left a comment (vim/vim#17978)Thinking about it a little more, I don’t think this change is really needed. Better not take the risk to cause more confusion.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#17978)Huh? There's no confusion or added risk. On the contrary, the code becomes easier to write and read, and we'll likely see fewer bugs.
Up until now, we've been using the int type in two ways: as a regular integer and as a Boolean (TRUE/FALSE).
Of course, the existing code works fine, but changing those Boolean uses to the bool type (true, false) would bring significant advantages.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.