Refactor: Simplify boolean conditional return statements in lib/StaticAnalyzer/Checkers

[LegalizeAdulthood]

Summary

Use clang-tidy to simplify boolean conditional return values

[LegalizeAdulthood]

Update to latest.
This changeset needs review; I am unsure who to add as specific reviewers.

[krememek]

These all look good to me.

[alexfh]

I assume, Richard doesn't have svn commit access yet. I'll commit the patch for him.

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

 /// If the fist block edge is a back edge, we are reentering the loop.
 static bool alreadyExecutedAtLeastOneLoopIteration(const ExplodedNode *N,
                                              const ObjCForCollectionStmt *FCS) {
   if (!N)
     return false;
 
   ProgramPoint P = N->getLocation();
   if (Optional<BlockEdge> BE = P.getAs<BlockEdge>()) {
-    if (BE->getSrc()->getLoopTarget() == FCS)
-      return true;
-    return false;
+    return BE->getSrc()->getLoopTarget() == FCS;
   }
 
   // Keep looking for a block edge.
   for (ExplodedNode::const_pred_iterator I = N->pred_begin(),
                                          E = N->pred_end(); I != E; ++I) {
     if (alreadyExecutedAtLeastOneLoopIteration(*I, FCS))
       return true;
   }

 bool ObjCLoopChecker::isCollectionCountMethod(const ObjCMethodCall &M,
                                               CheckerContext &C) const {
   Selector S = M.getSelector();
   // Initialize the identifiers on first use.
   if (!CountSelectorII)
     CountSelectorII = &C.getASTContext().Idents.get("count");
 
   // If the method returns collection count, record the value.
-  if (S.isUnarySelector() &&
-      (S.getIdentifierInfoForSlot(0) == CountSelectorII))
-    return true;
-  
-  return false;
+  return S.isUnarySelector() &&
+         (S.getIdentifierInfoForSlot(0) == CountSelectorII);
 }
 
 void ObjCLoopChecker::checkPostObjCMessage(const ObjCMethodCall &M,
                                            CheckerContext &C) const {
   if (!M.isInstanceMessage())
     return;
 
   const ObjCInterfaceDecl *ClassID = M.getReceiverInterface();

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
   (this->*evalFunction)(C, CE);
 
   // If the evaluate call resulted in no change, chain to the next eval call
   // handler.
   // Note, the custom CString evaluation calls assume that basic safety
   // properties are held. However, if the user chooses to turn off some of these
   // checks, we ignore the issues and leave the call evaluation to a generic
   // handler.
-  if (!C.isDifferent())
-    return false;
-
-  return true;
+  return C.isDifferent();
 }
 
 void CStringChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
   // Record string length for char a[] = "abc";
   ProgramStateRef state = C.getState();
 
   for (const auto *I : DS->decls()) {
     const VarDecl *D = dyn_cast<VarDecl>(I);

lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp

if (BE->getOpcode() == BO_Sub) {
         return true;
     }
   }
   // - sizeof(dst)
   if (isSizeof(LenArg, DstArg))
     return true;
 
   // - sizeof(src)
-  if (isSizeof(LenArg, SrcArg))
-    return true;
-  return false;
+  return isSizeof(LenArg, SrcArg);
 }
 
 void WalkAST::VisitCallExpr(CallExpr *CE) {
   const FunctionDecl *FD = CE->getDirectCallee();
   if (!FD)
     return;
 
   if (CheckerContext::isCLibraryFunction(FD, "strncat")) {

lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp

static bool evenFlexibleArraySize(ASTContext &Ctx, CharUnits RegionSize,
 
   if (FlexSize.isZero())
     return false;
 
   CharUnits Left = RegionSize - TypeSize;
   if (Left.isNegative())
     return false;
 
-  if (Left % FlexSize == 0)
-    return true;
-
-  return false;
+  return Left % FlexSize == 0;
 }
 
 void CastSizeChecker::checkPreStmt(const CastExpr *CE,CheckerContext &C) const {
   const Expr *E = CE->getSubExpr();
   ASTContext &Ctx = C.getASTContext();
   QualType ToTy = Ctx.getCanonicalType(CE->getType());
   const PointerType *ToPTy = dyn_cast<PointerType>(ToTy.getTypePtr());
 

lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp

 namespace {
 
 /// The default method filter, which is used to filter out the methods on which
 /// the check should not be performed.
 ///
 /// Checks for the init, dealloc, and any other functions that might be allowed
 /// to perform direct instance variable assignment based on their name.
 static bool DefaultMethodFilter(const ObjCMethodDecl *M) {
-  if (M->getMethodFamily() == OMF_init || M->getMethodFamily() == OMF_dealloc ||
+  return M->getMethodFamily() == OMF_init ||
+         M->getMethodFamily() == OMF_dealloc ||
          M->getMethodFamily() == OMF_copy ||
          M->getMethodFamily() == OMF_mutableCopy ||
          M->getSelector().getNameForSlot(0).find("init") != StringRef::npos ||
-      M->getSelector().getNameForSlot(0).find("Init") != StringRef::npos)
-    return true;
-  return false;
+         M->getSelector().getNameForSlot(0).find("Init") != StringRef::npos;
 }
 
 class DirectIvarAssignment :
   public Checker<check::ASTDecl<ObjCImplementationDecl> > {
 
   typedef llvm::DenseMap<const ObjCIvarDecl*,
                          const ObjCPropertyDecl*> IvarToPropertyMapTy;
 

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

bool GenericTaintChecker::checkPre(const CallExpr *CE, CheckerContext &C) const{
 
   StringRef Name = C.getCalleeName(FDecl);
   if (Name.empty())
     return false;
 
   if (checkSystemCall(CE, Name, C))
     return true;
 
-  if (checkTaintedBufferSize(CE, FDecl, C))
-    return true;
-
-  return false;
+  return checkTaintedBufferSize(CE, FDecl, C);
 }
 
 SymbolRef GenericTaintChecker::getPointedToSymbol(CheckerContext &C,
                                                   const Expr* Arg) {
   ProgramStateRef State = C.getState();
   SVal AddrVal = State->getSVal(Arg->IgnoreParens(), C.getLocationContext());
   if (AddrVal.isUnknownOrUndef())
     return nullptr;

 bool GenericTaintChecker::checkUncontrolledFormatString(const CallExpr *CE,
                                                         CheckerContext &C) const{
   // Check if the function contains a format string argument.
   unsigned int ArgNum = 0;
   if (!getPrintfFormatArgumentNum(CE, C, ArgNum))
     return false;
 
   // If either the format string content or the pointer itself are tainted, warn.
-  if (generateReportIfTainted(CE->getArg(ArgNum),
-                              MsgUncontrolledFormatString, C))
-    return true;
-  return false;
+  return generateReportIfTainted(CE->getArg(ArgNum),
+                                 MsgUncontrolledFormatString, C);
 }
 
 bool GenericTaintChecker::checkSystemCall(const CallExpr *CE,
                                           StringRef Name,
                                           CheckerContext &C) const {
   // TODO: It might make sense to run this check on demand. In some cases, 
   // we should check if the environment has been cleansed here. We also might 
   // need to know if the user was reset before these calls(seteuid).
   unsigned ArgNum = llvm::StringSwitch<unsigned>(Name)
     .Case("system", 0)
     .Case("popen", 0)
     .Case("execl", 0)
     .Case("execle", 0)
     .Case("execlp", 0)
     .Case("execv", 0)
     .Case("execvp", 0)
     .Case("execvP", 0)
     .Case("execve", 0)
     .Case("dlopen", 0)
     .Default(UINT_MAX);
 
   if (ArgNum == UINT_MAX || CE->getNumArgs() < (ArgNum + 1))
     return false;
 
-  if (generateReportIfTainted(CE->getArg(ArgNum),
-                              MsgSanitizeSystemArgs, C))
-    return true;
-
-  return false;
+  return generateReportIfTainted(CE->getArg(ArgNum), MsgSanitizeSystemArgs, C);
 }
 
 // TODO: Should this check be a part of the CString checker?
 // If yes, should taint be a global setting?
 bool GenericTaintChecker::checkTaintedBufferSize(const CallExpr *CE,
                                                  const FunctionDecl *FDecl,
                                                  CheckerContext &C) const {
   // If the function has a buffer size argument, set ArgNum.
if (ArgNum == InvalidArgIndex) {
     else if (C.isCLibraryFunction(FDecl, "memccpy"))
       ArgNum = 3;
     else if (C.isCLibraryFunction(FDecl, "realloc"))
       ArgNum = 1;
     else if (C.isCLibraryFunction(FDecl, "bcopy"))
       ArgNum = 2;
   }
 
-  if (ArgNum != InvalidArgIndex && CE->getNumArgs() > ArgNum &&
-      generateReportIfTainted(CE->getArg(ArgNum), MsgTaintedBufferSize, C))
-    return true;
-
-  return false;
+  return ArgNum != InvalidArgIndex && CE->getNumArgs() > ArgNum &&
+         generateReportIfTainted(CE->getArg(ArgNum), MsgTaintedBufferSize, C);
 }
 
 void ento::registerGenericTaintChecker(CheckerManager &mgr) {
   mgr.registerChecker<GenericTaintChecker>();
 }

lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp

 /// effects are never considered identical.
 /// Limitations: (t + u) and (u + t) are not considered identical.
 /// t*(u + t) and t*u + t*t are not considered identical.
 ///
 static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
                             const Stmt *Stmt2, bool IgnoreSideEffects) {
 
   if (!Stmt1 || !Stmt2) {
-    if (!Stmt1 && !Stmt2)
-      return true;
-    return false;
+    return !Stmt1 && !Stmt2;
   }
 
   // If Stmt1 & Stmt2 are of different class then they are not
   // identical statements.
   if (Stmt1->getStmtClass() != Stmt2->getStmtClass())
     return false;
 
   const Expr *Expr1 = dyn_cast<Expr>(Stmt1);
if (!isIdenticalStmt(Ctx, ForStmt1->getInit(), ForStmt2->getInit(),
                          IgnoreSideEffects))
       return false;
     if (!isIdenticalStmt(Ctx, ForStmt1->getCond(), ForStmt2->getCond(),
                          IgnoreSideEffects))
       return false;
     if (!isIdenticalStmt(Ctx, ForStmt1->getInc(), ForStmt2->getInc(),
                          IgnoreSideEffects))
       return false;
-    if (!isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
-                         IgnoreSideEffects))
-      return false;
-    return true;
+    return isIdenticalStmt(Ctx, ForStmt1->getBody(), ForStmt2->getBody(),
+                           IgnoreSideEffects);
   }
   case Stmt::DoStmtClass: {
     const DoStmt *DStmt1 = cast<DoStmt>(Stmt1);
     const DoStmt *DStmt2 = cast<DoStmt>(Stmt2);
 
     if (!isIdenticalStmt(Ctx, DStmt1->getCond(), DStmt2->getCond(),
                          IgnoreSideEffects))
       return false;
-    if (!isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
-                         IgnoreSideEffects))
-      return false;
-    return true;
+    return isIdenticalStmt(Ctx, DStmt1->getBody(), DStmt2->getBody(),
+                           IgnoreSideEffects);
   }
   case Stmt::WhileStmtClass: {
     const WhileStmt *WStmt1 = cast<WhileStmt>(Stmt1);
     const WhileStmt *WStmt2 = cast<WhileStmt>(Stmt2);
 
     if (!isIdenticalStmt(Ctx, WStmt1->getCond(), WStmt2->getCond(),
                          IgnoreSideEffects))
       return false;
-    if (!isIdenticalStmt(Ctx, WStmt1->getBody(), WStmt2->getBody(),
-                         IgnoreSideEffects))
-      return false;
-    return true;
+    return isIdenticalStmt(Ctx, WStmt1->getBody(), WStmt2->getBody(),
+                           IgnoreSideEffects);
   }
   case Stmt::IfStmtClass: {
     const IfStmt *IStmt1 = cast<IfStmt>(Stmt1);
     const IfStmt *IStmt2 = cast<IfStmt>(Stmt2);
 
     if (!isIdenticalStmt(Ctx, IStmt1->getCond(), IStmt2->getCond(),
                          IgnoreSideEffects))
       return false;
     if (!isIdenticalStmt(Ctx, IStmt1->getThen(), IStmt2->getThen(),
                          IgnoreSideEffects))
       return false;
-    if (!isIdenticalStmt(Ctx, IStmt1->getElse(), IStmt2->getElse(),
-                         IgnoreSideEffects))
-      return false;
-    return true;
+    return isIdenticalStmt(Ctx, IStmt1->getElse(), IStmt2->getElse(),
+                           IgnoreSideEffects);
   }
   case Stmt::CompoundStmtClass: {
     const CompoundStmt *CompStmt1 = cast<CompoundStmt>(Stmt1);
     const CompoundStmt *CompStmt2 = cast<CompoundStmt>(Stmt2);
 
     if (CompStmt1->size() != CompStmt2->size())
       return false;
 

lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

unsigned MacOSKeychainAPIChecker::getTrackedFunctionIndex(StringRef Name,
   }
   // The function is not tracked.
   return InvalidIdx;
 }
 
 static bool isBadDeallocationArgument(const MemRegion *Arg) {
   if (!Arg)
     return false;
-  if (isa<AllocaRegion>(Arg) ||
-      isa<BlockDataRegion>(Arg) ||
-      isa<TypedRegion>(Arg)) {
-    return true;
-  }
-  return false;
+  return isa<AllocaRegion>(Arg) || isa<BlockDataRegion>(Arg) ||
+         isa<TypedRegion>(Arg);
 }
 
 /// Given the address expression, retrieve the value it's pointing to. Assume
 /// that value is itself an address, and return the corresponding symbol.
 static SymbolRef getAsPointeeSymbol(const Expr *Expr,
                                     CheckerContext &C) {
   ProgramStateRef State = C.getState();
   SVal ArgV = State->getSVal(Expr, C.getLocationContext());
bool MacOSKeychainAPIChecker::definitelyReturnedError(SymbolRef RetSym,
                                                       ProgramStateRef State,
                                                       SValBuilder &Builder,
                                                       bool noError) const {
   DefinedOrUnknownSVal NoErrVal = Builder.makeIntVal(NoErr,
     Builder.getSymbolManager().getType(RetSym));
   DefinedOrUnknownSVal NoErr = Builder.evalEQ(State, NoErrVal,
                                                      nonloc::SymbolVal(RetSym));
   ProgramStateRef ErrState = State->assume(NoErr, noError);
-  if (ErrState == State) {
-    return true;
-  }
-
-  return false;
+  return ErrState == State;
 }
 
 // Report deallocator mismatch. Remove the region from tracking - reporting a
 // missing free error after this one is redundant.
 void MacOSKeychainAPIChecker::
   generateDeallocatorMismatchReport(const AllocationPair &AP,
                                     const Expr *ArgExpr,
                                     CheckerContext &C) const {

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

if (isCMemFunction(FD, C, AF_Malloc, MemoryOperationKind::MOK_Any))
     return true;
 
   if (isCMemFunction(FD, C, AF_IfNameIndex, MemoryOperationKind::MOK_Any))
     return true;
 
   if (isCMemFunction(FD, C, AF_Alloca, MemoryOperationKind::MOK_Any))
     return true;
 
-  if (isStandardNewDelete(FD, C))
-    return true;
-
-  return false;
+  return isStandardNewDelete(FD, C);
 }
 
 bool MallocChecker::isCMemFunction(const FunctionDecl *FD,
                                    ASTContext &C,
                                    AllocationFamily Family,
                                    MemoryOperationKind MemKind) const {
   if (!FD)
     return false;

 
 static bool isKnownDeallocObjCMethodName(const ObjCMethodCall &Call) {
   // If the first selector piece is one of the names below, assume that the
   // object takes ownership of the memory, promising to eventually deallocate it
   // with free().
   // Ex:  [NSData dataWithBytesNoCopy:bytes length:10];
   // (...unless a 'freeWhenDone' parameter is false, but that's checked later.)
   StringRef FirstSlot = Call.getSelector().getNameForSlot(0);
-  if (FirstSlot == "dataWithBytesNoCopy" ||
+  return FirstSlot == "dataWithBytesNoCopy" ||
          FirstSlot == "initWithBytesNoCopy" ||
-      FirstSlot == "initWithCharactersNoCopy")
-    return true;
-
-  return false;
+         FirstSlot == "initWithCharactersNoCopy";
 }
 
 static Optional<bool> getFreeWhenDoneArg(const ObjCMethodCall &Call) {
   Selector S = Call.getSelector();
 
   // FIXME: We should not rely on fully-constrained symbols being folded.
   for (unsigned i = 1; i < S.getNumArgs(); ++i)
     if (S.getNameForSlot(i).equals("freeWhenDone"))

lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp

inline bool hasPointerToPointerSizedType(const Expr *E) {
     }
 
     if (const Type *TElem = TP->getArrayElementTypeNoTypeQual())
       return isPointerSize(TElem);
 
     // The type must be an array/pointer type.
 
     // This could be a null constant, which is allowed.
-    if (E->isNullPointerConstant(ASTC, Expr::NPC_ValueDependentIsNull))
-      return true;
-    return false;
+    return E->isNullPointerConstant(ASTC, Expr::NPC_ValueDependentIsNull);
   }
 
 public:
   WalkAST(BugReporter &br, const CheckerBase *checker, AnalysisDeclContext *ac)
       : BR(br), Checker(checker), AC(ac), ASTC(AC->getASTContext()),
         PtrWidth(ASTC.getTargetInfo().getPointerWidth(0)) {}
 
   // Statement visitor methods.

lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp

static bool shouldRunOnFunctionOrMethod(const NamedDecl *ND) {
   IdentifierInfo* NSObjectII = &Ctx.Idents.get("NSObject");
   ObjCInterfaceDecl *ID = MD->getClassInterface()->getSuperClass();
   for ( ; ID ; ID = ID->getSuperClass()) {
     IdentifierInfo *II = ID->getIdentifier();
 
     if (II == NSObjectII)
       break;
   }
-  if (!ID)
-    return false;
-
-  return true;
+  return ID;
 }
 
 /// \brief Returns true if the location is 'self'.
 static bool isSelfVar(SVal location, CheckerContext &C) {
   AnalysisDeclContext *analCtx = C.getCurrentAnalysisDeclContext(); 
   if (!analCtx->getSelfDecl())
     return false;
   if (!location.getAs<loc::MemRegionVal>())

lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp

bool UnreachableCodeChecker::isInvalidPath(const CFGBlock *CB,
 
   //assert(cond && "CFGBlock's predecessor has a terminator condition");
   // The previous assertion is invalid in some cases (eg do/while). Leaving
   // reporting of these situations on at the moment to help triage these cases.
   if (!cond)
     return false;
 
   // Run each of the checks on the conditions
-  if (containsMacro(cond) || containsEnum(cond)
-      || containsStaticLocal(cond) || containsBuiltinOffsetOf(cond)
-      || containsStmt<UnaryExprOrTypeTraitExpr>(cond))
-    return true;
-
-  return false;
+  return containsMacro(cond) || containsEnum(cond) ||
+         containsStaticLocal(cond) || containsBuiltinOffsetOf(cond) ||
+         containsStmt<UnaryExprOrTypeTraitExpr>(cond);
 }
 
 // Returns true if the given CFGBlock is empty
 bool UnreachableCodeChecker::isEmptyCFGBlock(const CFGBlock *CB) {
   return CB->getLabel() == nullptr // No labels
       && CB->size() == 0           // No statements
       && !CB->getTerminator();     // No terminator
 }
 
 void ento::registerUnreachableCodeChecker(CheckerManager &mgr) {
   mgr.registerChecker<UnreachableCodeChecker>();
 }