[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] Enhance clang-tidy readability-simplify-boolean-expr to handle 'if (e) return true; retu
From:       Richard <legalize () xmission ! com>
Date:       2015-05-31 3:31:54
Message-ID: eb8b18ca16333b7d8d3826a0aa38875d () localhost ! localdomain
[Download RAW message or body]

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:134
@@ +133,3 @@
+                     const CXXBoolLiteralExpr *&Lit) {
+  if (auto *Bool = dyn_cast<CXXBoolLiteralExpr>(Ret->getRetValue())) {
+    Lit = Bool;
----------------
alexfh wrote:
> Please use `const auto *` to make the constness explicit.
Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:141
@@ +140,3 @@
+
+bool stmtReturnsBool(IfStmt *IfRet, bool Negated,
+                     const CXXBoolLiteralExpr *&Lit) {
----------------
alexfh wrote:
> Instead of using the return value and additionally return a literal by reference, \
> I'd suggest making the function return `llvm::Optional<const CXXBoolLiteralExpr*>`. \
>  Same above.
Any reason to not return just the pointer and compare against `nulltpr`?

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:147
@@ +146,3 @@
+
+  if (auto *Ret = dyn_cast<ReturnStmt>(IfRet->getThen())) {
+    return stmtReturnsBool(Ret, Negated, Lit);
----------------
alexfh wrote:
> Please use `const auto *` to make the constness explicit. Also two times below.
Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:149
@@ +148,3 @@
+    return stmtReturnsBool(Ret, Negated, Lit);
+  } else if (auto *Compound = dyn_cast<CompoundStmt>(IfRet->getThen())) {
+    if (Compound->size() == 1) {
----------------
alexfh wrote:
> nit: No `else` after `return` please.
Fixed.

================
Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:451
@@ +450,3 @@
+       ++BeforeIt, ++AfterIt) {
+    if (auto *If = dyn_cast<IfStmt>(*BeforeIt)) {
+      const CXXBoolLiteralExpr *Lit = nullptr;
----------------
alexfh wrote:
> Please use `const auto *` to make the constness explicit.
Fixed.

================
Comment at: test/clang-tidy/readability-simplify-bool-expr.cpp:704
@@ +703,3 @@
+// CHECK-FIXES: {{^}}    return false;{{$}}
+// CHECK-FIXES: {{^}}  }{{$}}
+// CHECK-FIXES: {{^  return i <= 10;$}}
----------------
alexfh wrote:
> How about making it a single regexp?
> 
> {{^  }$}}
> 
> Same applies for other similar lines.
Fixed.

http://reviews.llvm.org/D9810

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]