1536383 - [Automated review] clang-tidy readability-simplify-boolean-expr is often not an improvement

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Jan de Mooij [:jandem]]

Phabricator URL: https://phabricator.services.mozilla.com/D21528

In the JS engine we often write code like this:

if (tryFoo()) {
  return true;
}
if (tryBar()) {
  return true;
}
return false;

clang-tidy then warns
Warning: Redundant boolean literal in conditional return statement [clang-tidy: readability-simplify-boolean-expr]

It wants us to change the code to:

if (tryFoo()) {
  return true;
}
return tryBar();

However that's worse because it breaks the "pattern" and if you want to add tryBaz() you now have to change more lines of code.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

I prefer the later code snippet but this is a different discussion.
Should we disable this checker for the js engine?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #1)

Should we disable this checker for the js engine?

I'm not sure. Looking at the documentation [0] there are other, more useful, things this check will catch, so maybe we should just cope with it...

CC'ing some people who might have thoughts on this.

[0] https://clang.llvm.org/extra/clang-tidy/checks/readability-simplify-boolean-expr.html

Comment 3

[Ted Campbell [:tcampbell]]

My impression was that the |return tryBar| was always the SpiderMonkey coding style. I adjusted to it when I started the project and have come to find it part of the "pattern" and it does save a few lines. I find with the tail-call-style syntax that I'm less likely to forget the final return statement (although that is easily caught by compilers/our tooling).

Based on personal (subjective) preference and existing code, I'd like to keep the checker.

Comment 4

[Jan de Mooij [:jandem]]

OK probably not worth doing anything here after all.