| Issue |
164896
|
| Summary |
readability-simplify-boolean-expr often suggests changes that harm readability for `if (cond) { return true; } return false;`
|
| Labels |
new issue
|
| Assignees |
|
| Reporter |
zygoloid
|
Consider:
```c++
if (thing1) {
return true;
}
if (thing2) {
return true;
}
for (x : list) {
if (thing3(x)) {
return true;
}
}
if (thing4) {
return true;
}
return false;
```
This seems reasonable and readable: we have a bunch of parallel cases that we want to check. But readability-simplify-boolean-expr wants the end of this to be rewritten as:
```c++
return thing4;
```
This change makes readability worse, because `thing4` is no longer expressed in a way that is parallel to the other checks for `thing1`-`thing3`. This makes the code less self-consistent, which is an important aspect of readability -- making similar things look similar and making different things look different. It also makes the function harder to maintain, because this transformation now needs to be undone when a check for `thing5` is added.
We should find a simple heuristic to distinguish this from cases where the warning is useful. Perhaps we could suppress the warning if the block scope containing the `if (cond) { return true; } return false;` contains any other `return` statements? I think that would suppress all the bad suggestions of this kind I've seen without suppressing any of the good ones.
![]()
_______________________________________________
llvm-bugs mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-bugs
