Refactor: Simplify boolean conditional return statements in lib/Edit

[LegalizeAdulthood]

Summary

Use clang-tidy to simplify boolean conditional return statements

[calabrese]

I like the idea of all of these changes (this change list and the others), but just a suggestion -- it looks like when the branch of the original condition returns false, as was the case in the original code here, the internal expressions of the check are transformed via De Morgan's laws in the new code. IMO, this transformation sometimes makes things more complicated rather than less, where by "more complicated" I mean that the transformation produces an overall expression that is constructed from a considerably greater number of logical operations. For instance, here it changed a bunch of || expressions into a bunch of && expressions, with each operand individually prefixed with "!". The equivalent without having applied De Morgan's laws would just have left all of the || expressions as-is and added a single ! around the overall expression parenthesized. It would have also looked much more similar to the original code.

My general thought here is that it might make sense when performing this kind of transformation to first see if the translation via De Morgan's laws actually produces an overall expression that has fewer logical subexpressions, otherwise prefer the minimal transformation of a ! around a parenthesized expression.

[dblaikie]

lib/Edit/RewriteObjCFoundationAPI.cpp

634

Looks like a chain (see prior review on your changes for more detail on what I mean by this)

[LegalizeAdulthood]

Update from latest.
I do not have commit access.

[LegalizeAdulthood]

In D10014#178294, @calabrese wrote:

I like the idea of all of these changes (this change list and the others), but just a suggestion -- it looks like when the branch of the original condition returns false, as was the case in the original code here, the internal expressions of the check are transformed via De Morgan's laws in the new code. IMO, this transformation sometimes makes things more complicated rather than less, where by "more complicated" I mean that the transformation produces an overall expression that is constructed from a considerably greater number of logical operations. For instance, here it changed a bunch of || expressions into a bunch of && expressions, with each operand individually prefixed with "!". The equivalent without having applied De Morgan's laws would just have left all of the || expressions as-is and added a single ! around the overall expression parenthesized. It would have also looked much more similar to the original code.

My general thought here is that it might make sense when performing this kind of transformation to first see if the translation via De Morgan's laws actually produces an overall expression that has fewer logical subexpressions, otherwise prefer the minimal transformation of a ! around a parenthesized expression.

This should be addressed in the latest diff.

lib/Edit/RewriteObjCFoundationAPI.cpp

if (Sel == NS.getNSDictionarySelector(
 
     SmallVector<const Expr *, 8> Keys;
     if (!getNSArrayObjects(Msg->getArg(1), NS, Keys))
       return false;
 
     if (Vals.size() != Keys.size())
       return false;
 
     return true;
   }
 
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // rewriteToNumberLiteral.
 //===----------------------------------------------------------------------===//
static bool rewriteToNumberLiteral(const ObjCMessageExpr *Msg,
   }
   return true;
 }
 
 // FIXME: Make determination of operator precedence more general and
 // make it broadly available.
 static bool subscriptOperatorNeedsParens(const Expr *FullExpr) {
   const Expr* Expr = FullExpr->IgnoreImpCasts();
-  if (isa<ArraySubscriptExpr>(Expr) ||
-      isa<CallExpr>(Expr) ||
-      isa<DeclRefExpr>(Expr) ||
-      isa<CXXNamedCastExpr>(Expr) ||
-      isa<CXXConstructExpr>(Expr) ||
-      isa<CXXThisExpr>(Expr) ||
-      isa<CXXTypeidExpr>(Expr) ||
-      isa<CXXUnresolvedConstructExpr>(Expr) ||
-      isa<ObjCMessageExpr>(Expr) ||
-      isa<ObjCPropertyRefExpr>(Expr) ||
-      isa<ObjCProtocolExpr>(Expr) ||
-      isa<MemberExpr>(Expr) ||
-      isa<ObjCIvarRefExpr>(Expr) ||
-      isa<ParenExpr>(FullExpr) ||
-      isa<ParenListExpr>(Expr) ||
-      isa<SizeOfPackExpr>(Expr))
-    return false;
-
-  return true;
+  return !(isa<ArraySubscriptExpr>(Expr) || isa<CallExpr>(Expr) ||
+           isa<DeclRefExpr>(Expr) || isa<CXXNamedCastExpr>(Expr) ||
+           isa<CXXConstructExpr>(Expr) || isa<CXXThisExpr>(Expr) ||
+           isa<CXXTypeidExpr>(Expr) || isa<CXXUnresolvedConstructExpr>(Expr) ||
+           isa<ObjCMessageExpr>(Expr) || isa<ObjCPropertyRefExpr>(Expr) ||
+           isa<ObjCProtocolExpr>(Expr) || isa<MemberExpr>(Expr) ||
+           isa<ObjCIvarRefExpr>(Expr) || isa<ParenExpr>(FullExpr) ||
+           isa<ParenListExpr>(Expr) || isa<SizeOfPackExpr>(Expr));
 }
 static bool castOperatorNeedsParens(const Expr *FullExpr) {
   const Expr* Expr = FullExpr->IgnoreImpCasts();
-  if (isa<ArraySubscriptExpr>(Expr) ||
-      isa<CallExpr>(Expr) ||
-      isa<DeclRefExpr>(Expr) ||
-      isa<CastExpr>(Expr) ||
-      isa<CXXNewExpr>(Expr) ||
-      isa<CXXConstructExpr>(Expr) ||
-      isa<CXXDeleteExpr>(Expr) ||
-      isa<CXXNoexceptExpr>(Expr) ||
+  return !(isa<ArraySubscriptExpr>(Expr) || isa<CallExpr>(Expr) ||
+           isa<DeclRefExpr>(Expr) || isa<CastExpr>(Expr) ||
+           isa<CXXNewExpr>(Expr) || isa<CXXConstructExpr>(Expr) ||
+           isa<CXXDeleteExpr>(Expr) || isa<CXXNoexceptExpr>(Expr) ||
            isa<CXXPseudoDestructorExpr>(Expr) ||
-      isa<CXXScalarValueInitExpr>(Expr) ||
-      isa<CXXThisExpr>(Expr) ||
-      isa<CXXTypeidExpr>(Expr) ||
-      isa<CXXUnresolvedConstructExpr>(Expr) ||
-      isa<ObjCMessageExpr>(Expr) ||
-      isa<ObjCPropertyRefExpr>(Expr) ||
-      isa<ObjCProtocolExpr>(Expr) ||
-      isa<MemberExpr>(Expr) ||
-      isa<ObjCIvarRefExpr>(Expr) ||
-      isa<ParenExpr>(FullExpr) ||
-      isa<ParenListExpr>(Expr) ||
-      isa<SizeOfPackExpr>(Expr) ||
-      isa<UnaryOperator>(Expr))
-    return false;
-
-  return true;
+           isa<CXXScalarValueInitExpr>(Expr) || isa<CXXThisExpr>(Expr) ||
+           isa<CXXTypeidExpr>(Expr) || isa<CXXUnresolvedConstructExpr>(Expr) ||
+           isa<ObjCMessageExpr>(Expr) || isa<ObjCPropertyRefExpr>(Expr) ||
+           isa<ObjCProtocolExpr>(Expr) || isa<MemberExpr>(Expr) ||
+           isa<ObjCIvarRefExpr>(Expr) || isa<ParenExpr>(FullExpr) ||
+           isa<ParenListExpr>(Expr) || isa<SizeOfPackExpr>(Expr) ||
+           isa<UnaryOperator>(Expr));
 }
 
 static void objectifyExpr(const Expr *E, Commit &commit) {
   if (!E) return;
 
   QualType T = E->getType();
   if (T->isObjCObjectPointerType()) {
     if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {