readability-simplify-boolean-expr often suggests changes that harm readability for `if (cond) { return true; } return false;`

[zygoloid]

Consider:

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:

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.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Richard Smith (zygoloid)

Consider:

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:

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.