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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to