[PATCH] D56323: [clang-tidy] Handle member variables in readability-simplify-boolean-expr

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 5 inline comments as done.
LegalizeAdulthood added inline comments.


================
Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:4
+struct X {
+  explicit operator bool();
+};
----------------
JonasToth wrote:
> LegalizeAdulthood wrote:
> > JonasToth wrote:
> > > I didn't see a test that utilized this struct, did I overlook it?
> > > 
> > > Implicit conversion to `bool`-cases would be interesting as well. Maybe implicit conversion to integral in general.
> > Leftover from copy/paste of readability-simplify-bool-expr.cpp, I'll remove it.
> > 
> > Implicit conversion cases are covered by the other file.  Here, I'm just testing the cases that interact with member variables because the matcher needs to use memberExpr() and not just declRefExpr()
> Which other file? The other revision?
I based this new file off readability-simplify-bool-expr.cpp that is already there.  It has all the main test cases for this check.


================
Comment at: test/clang-tidy/readability-simplify-bool-expr-members.cpp:160
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: {{.*}} in ternary expression result
+  // CHECK-FIXES: {{^  m_b2 = i <= 20;$}}
+
----------------
JonasToth wrote:
> LegalizeAdulthood wrote:
> > JonasToth wrote:
> > > For this case the logical inversion is simple an obviously `<=`, but what if the condition is more complex?
> > > 
> > > I would personally prefer `!(i < 20)` instead + complex logical expressions as additional test. They would be interesting for the `if` cases, too.
> > If you have complex conditions, you can run the check repeatedly until you reach a fixed point.
> > 
> > Using !(x) instead of the reverse operator could be added as an enhancement, but it is not covered by this patch.
> But the current version is the enhancement.
> The complex condition, like `i < 20 && i > 0` will be the common case, so the inversion (with parens) should always be used (`!(i < 20 && i > 0)`) and specializations can be done later.
No, the current version is a bug fix.  The simplification of boolean expressions was already there, but it didn't handle member variables properly.

You're asking for an additional enhancement about the way boolean expressions are simplified.  That's fine for an additional enhancement of the check, but it is not the point of this patch.

The complex expression you're discussing in your comment is not changed by this change, nor is it changed by the existing check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56323/new/

https://reviews.llvm.org/D56323