Refactor: Simplify boolean conditional return statements in lib/Sema

[LegalizeAdulthood]

Summary

Use clang-tidy to simplify boolean conditional return statements

[dblaikie]

Got only part way through this one.

We should come to some conclusion about these chained return cases - they seem fairly benign/not buggy/not worth changing.

Basically what I mean when I say "chain" or "possible chain" is that it looks like this last if/return is just another case of several in the same function and shouldn't be treated differently by being rolled into one final return statement. Mostly the question I think that's worth asking is "could I reasonably add another condition on to the end of this function?" and if so, it hurts readability/writability by rolling it in and making it somehow different from the other cases in the function.

I think this was a concern John McCall raised a few weeks ago on the same sort of changes you'd been proposing.

lib/Sema/SemaChecking.cpp

5019

Possible chain

9131

Possible chain

9229

Possible chain

lib/Sema/SemaDecl.cpp

2556

Possible chain

14011

Possible chain

lib/Sema/SemaDeclAttr.cpp

367

possible chain

460

Possible chain

1816

possible chain

[LegalizeAdulthood]

In D10019#178722, @dblaikie wrote:

We should come to some conclusion about these chained return cases - they seem fairly benign/not buggy/not worth changing.

Ah... I see what you mean. So like the other case where it was something like if (expr) var = true; else if (expr2) var = true; else if (expr3) var = true; else var = false;, you'd like the option of disabling this check when it is a series of chained return statements?

That's great feedback and is something I can do to enhance the check.

[LegalizeAdulthood]

Update from latest.
Update from comments.
I do not have commit access.

[alexfh]

David, have your comments been addressed completely?

lib/Sema/SemaChecking.cpp

static bool IsFunctionStdAbs(const FunctionDecl *FDecl) {
   }
 
   if (!ND || !ND->getIdentifier() || !ND->getIdentifier()->isStr("std"))
     return false;
 
   if (!isa<TranslationUnitDecl>(ND->getDeclContext()))
     return false;
 
   return true;
 }
 
 // Warn when using the wrong abs() function.
 void Sema::CheckAbsoluteValueFunction(const CallExpr *Call,
                                       const FunctionDecl *FDecl,
                                       IdentifierInfo *FnInfo) {
   if (Call->getNumArgs() != 1)
     return;
if (cast->getCastKind() == CK_ARCConsumeObject) {
         << (LT == Qualifiers::OCL_ExplicitNone)
         << (isProperty ? 0 : 1)
         << RHS->getSourceRange();
       return true;
     }
     RHS = cast->getSubExpr();
   }
 
-  if (LT == Qualifiers::OCL_Weak &&
-      checkUnsafeAssignLiteral(S, Loc, RHS, isProperty))
-    return true;
-
-  return false;
+  return LT == Qualifiers::OCL_Weak &&
+         checkUnsafeAssignLiteral(S, Loc, RHS, isProperty);
 }
 
 bool Sema::checkUnsafeAssigns(SourceLocation Loc,
                               QualType LHS, Expr *RHS) {
   Qualifiers::ObjCLifetime LT = LHS.getObjCLifetime();
 
   if (LT != Qualifiers::OCL_Weak && LT != Qualifiers::OCL_ExplicitNone)
     return false;
 
-  if (checkUnsafeAssignObject(*this, Loc, LT, RHS, false))
-    return true;
-
-  return false;
+  return checkUnsafeAssignObject(*this, Loc, LT, RHS, false);
 }
 
 void Sema::checkUnsafeExprAssigns(SourceLocation Loc,
                               Expr *LHS, Expr *RHS) {
   QualType LHSType;
   // PropertyRef on LHS type need be directly obtained from
   // its declaration as it has a PseudoType.
   ObjCPropertyRefExpr *PRE
unsigned BodyLine = SourceMgr.getSpellingLineNumber(Body->getSemiLoc(),
                                                       &BodyLineInvalid);
   if (BodyLineInvalid)
     return false;
 
   // Warn if null statement and body are on the same line.
   if (StmtLine != BodyLine)
     return false;
 
   return true;
 }
 } // Unnamed namespace
 
 void Sema::DiagnoseEmptyStmtBody(SourceLocation StmtLoc,
                                  const Stmt *Body,
                                  unsigned DiagID) {
   // Since this is a syntactic check, don't emit diagnostic for template
   // instantiations, this just adds noise.
bool isLayoutCompatibleStruct(ASTContext &C,
   RecordDecl::field_iterator Field2 = RD2->field_begin(),
                              Field2End = RD2->field_end(),
                              Field1 = RD1->field_begin(),
                              Field1End = RD1->field_end();
   for ( ; Field1 != Field1End && Field2 != Field2End; ++Field1, ++Field2) {
     if (!isLayoutCompatible(C, *Field1, *Field2))
       return false;
   }
-  if (Field1 != Field1End || Field2 != Field2End)
-    return false;
-
-  return true;
+  return !(Field1 != Field1End || Field2 != Field2End);
 }
 
 /// \brief Check if two standard-layout unions are layout-compatible.
 /// (C++11 [class.mem] p18)
 bool isLayoutCompatibleUnion(ASTContext &C,
                              RecordDecl *RD1,
                              RecordDecl *RD2) {
   llvm::SmallPtrSet<FieldDecl *, 8> UnmatchedFields;

lib/Sema/SemaCodeComplete.cpp

static bool ObjCPropertyFlagConflicts(unsigned Attributes, unsigned NewFlag) {
   
   // Check for more than one of { assign, copy, retain, strong, weak }.
   unsigned AssignCopyRetMask = Attributes & (ObjCDeclSpec::DQ_PR_assign |
                                          ObjCDeclSpec::DQ_PR_unsafe_unretained |
                                              ObjCDeclSpec::DQ_PR_copy |
                                              ObjCDeclSpec::DQ_PR_retain |
                                              ObjCDeclSpec::DQ_PR_strong |
                                              ObjCDeclSpec::DQ_PR_weak);
-  if (AssignCopyRetMask &&
-      AssignCopyRetMask != ObjCDeclSpec::DQ_PR_assign &&
+  return AssignCopyRetMask && AssignCopyRetMask != ObjCDeclSpec::DQ_PR_assign &&
          AssignCopyRetMask != ObjCDeclSpec::DQ_PR_unsafe_unretained &&
          AssignCopyRetMask != ObjCDeclSpec::DQ_PR_copy &&
          AssignCopyRetMask != ObjCDeclSpec::DQ_PR_retain &&
          AssignCopyRetMask != ObjCDeclSpec::DQ_PR_strong &&
-      AssignCopyRetMask != ObjCDeclSpec::DQ_PR_weak)
-    return true;
-  
-  return false;
+         AssignCopyRetMask != ObjCDeclSpec::DQ_PR_weak;
 }
 
 void Sema::CodeCompleteObjCPropertyFlags(Scope *S, ObjCDeclSpec &ODS) { 
   if (!CodeCompleter)
     return;
   
   unsigned Attributes = ODS.getPropertyAttributes();
   

lib/Sema/SemaDecl.cpp

static bool haveIncompatibleLanguageLinkages(const T *Old, const T *New) {
   if (DC->isRecord())
     return false;
 
   LanguageLinkage OldLinkage = Old->getLanguageLinkage();
   if (OldLinkage == CXXLanguageLinkage && New->isInExternCContext())
     return true;
   if (OldLinkage == CLanguageLinkage && New->isInExternCXXContext())
     return true;
   return false;
 }
 
 template<typename T> static bool isExternC(T *D) { return D->isExternC(); }
 static bool isExternC(VarTemplateDecl *) { return false; }
 
 /// \brief Check whether a redeclaration of an entity introduced by a
 /// using-declaration is valid, given that we know it's not an overload
 /// (nor a hidden tag declaration).
if (Cur->isRecord()) {
     // Cannot qualify members within a class.
     Diag(Loc, diag::err_member_qualification)
       << Name << SS.getRange();
     SS.clear();
     
     // C++ constructors and destructors with incorrect scopes can break
     // our AST invariants by having the wrong underlying types. If
     // that's the case, then drop this declaration entirely.
-    if ((Name.getNameKind() == DeclarationName::CXXConstructorName ||
+    return (Name.getNameKind() == DeclarationName::CXXConstructorName ||
             Name.getNameKind() == DeclarationName::CXXDestructorName) &&
-        !Context.hasSameType(Name.getCXXNameType(),
-                             Context.getTypeDeclType(cast<CXXRecordDecl>(Cur))))
-      return true;
-    
-    return false;
+           !Context.hasSameType(
+               Name.getCXXNameType(),
+               Context.getTypeDeclType(cast<CXXRecordDecl>(Cur)));
   }
   
   // C++11 [dcl.meaning]p1:
   //   [...] "The nested-name-specifier of the qualified declarator-id shall
   //   not begin with a decltype-specifer"
   NestedNameSpecifierLoc SpecLoc(SS.getScopeRep(), SS.location_data());
   while (SpecLoc.getPrefix())
     SpecLoc = SpecLoc.getPrefix();
static bool ValidDuplicateEnum(EnumConstantDecl *ECD, EnumDecl *Enum) {
   EnumConstantDecl *EnumConstant = dyn_cast<EnumConstantDecl>(DRE->getDecl());
   if (!EnumConstant)
     return true;
 
   if (cast<EnumDecl>(TagDecl::castFromDeclContext(ECD->getDeclContext())) !=
       Enum)
     return true;
 
   return false;
 }
 
 struct DupKey {
   int64_t val;
   bool isTombstoneOrEmptyKey;
   DupKey(int64_t val, bool isTombstoneOrEmptyKey)
     : val(val), isTombstoneOrEmptyKey(isTombstoneOrEmptyKey) {}
 };

lib/Sema/SemaDeclAttr.cpp

 static bool threadSafetyCheckIsSmartPointer(Sema &S, const RecordType* RT) {
   DeclContextLookupResult Res1 = RT->getDecl()->lookup(
       S.Context.DeclarationNames.getCXXOperatorName(OO_Star));
   if (Res1.empty())
     return false;
 
   DeclContextLookupResult Res2 = RT->getDecl()->lookup(
       S.Context.DeclarationNames.getCXXOperatorName(OO_Arrow));
-  if (Res2.empty())
-    return false;
-
-  return true;
+  return !Res2.empty();
 }
 
 /// \brief Check if passed in Decl is a pointer type.
 /// Note that this function may produce an error message.
 /// \return true if the Decl is a pointer type; false otherwise
 static bool threadSafetyCheckIsPointer(Sema &S, const Decl *D,
                                        const AttributeList &Attr) {
   const ValueDecl *vd = cast<ValueDecl>(D);

 
 static bool typeHasCapability(Sema &S, QualType Ty) {
   if (checkTypedefTypeForCapability(Ty))
     return true;
 
   if (checkRecordTypeForCapability(S, Ty))
     return true;
 
   return false;
 }
 
 static bool isCapabilityExpr(Sema &S, const Expr *Ex) {
   // Capability expressions are simple expressions involving the boolean logic
   // operators &&, || or !, a simple DeclRefExpr, CastExpr or a ParenExpr. Once
   // a DeclRefExpr is found, its type should be checked to determine whether it
   // is a capability or not.
 
static bool checkAcquireOrderAttrCommon(Sema &S, Decl *D,
   if (!QT->isDependentType() && !typeHasCapability(S, QT)) {
     S.Diag(Attr.getLoc(), diag::warn_thread_attribute_decl_not_lockable)
       << Attr.getName();
     return false;
   }
 
   // Check that all arguments are lockable objects.
   checkAttrArgsAreCapabilityObjs(S, D, Attr, Args);
-  if (Args.empty())
-    return false;
-
-  return true;
+  return !Args.empty();
 }
 
 static void handleAcquiredAfterAttr(Sema &S, Decl *D,
                                     const AttributeList &Attr) {
   SmallVector<Expr*, 1> Args;
   if (!checkAcquireOrderAttrCommon(S, D, Attr, Args))
     return;
 
if (X.empty() || Y.empty())
     return true;
 
   if (X == Y)
     return true;
 
   if (BeforeIsOkay && X < Y)
     return true;
 
   return false;
 }
 
 AvailabilityAttr *Sema::mergeAvailabilityAttr(NamedDecl *D, SourceRange Range,
                                               IdentifierInfo *Platform,
                                               VersionTuple Introduced,
                                               VersionTuple Deprecated,
                                               VersionTuple Obsoleted,
                                               bool IsUnavailable,

lib/Sema/SemaDeclCXX.cpp

if (!isa<CXXConstructorDecl>(NewFD)) {
     QualType RT = NewFD->getReturnType();
     if (!RT->isDependentType() &&
         RequireLiteralType(NewFD->getLocation(), RT,
                            diag::err_constexpr_non_literal_return))
       return false;
   }
 
   // - each of its parameter types shall be a literal type;
-  if (!CheckConstexprParameterTypes(*this, NewFD))
-    return false;
-
-  return true;
+  return CheckConstexprParameterTypes(*this, NewFD);
 }
 
 /// Check the given declaration statement is legal within a constexpr function
 /// body. C++11 [dcl.constexpr]p3,p4, and C++1y [dcl.constexpr]p3.
 ///
 /// \return true if the body is OK (maybe only as an extension), false if we
 ///         have diagnosed a problem.
 static bool CheckConstexprDeclStmt(Sema &SemaRef, const FunctionDecl *Dcl,

lib/Sema/SemaDeclObjC.cpp

if (LookupKind != Sema::LookupObjCProtocolName) {
         // can happy because we look for any name when there is no
         // bias to protocol or type names.
         if (isa<RecordDecl>(typeDecl) && !Context.getLangOpts().CPlusPlus)
           return false;
 
         // Make sure the type is something we would accept as a type
         // argument.
         auto type = Context.getTypeDeclType(typeDecl);
-        if (type->isObjCObjectPointerType() ||
-            type->isBlockPointerType() ||
-            type->isDependentType() ||
-            type->isObjCObjectType())
-          return true;
-
-        return false;
+        return type->isObjCObjectPointerType() || type->isBlockPointerType() ||
+               type->isDependentType() || type->isObjCObjectType();
       }
 
       // If we have an Objective-C class type, accept it; there will
       // be another fix to add the '*'.
-      if (candidate.getCorrectionDeclAs<ObjCInterfaceDecl>())
-        return true;
-
-      return false;
+      return candidate.getCorrectionDeclAs<ObjCInterfaceDecl>() != nullptr;
     }
 
     return false;
   }
 };
 } // end anonymous namespace
 
 void Sema::actOnObjCTypeArgsOrProtocolQualifiers(

lib/Sema/SemaExceptionSpec.cpp

bool Sema::CheckSpecifiedExceptionType(QualType &T, SourceRange Range) {
   }
 
   // C++11 [except.spec]p2:
   //   A type denoted in an exception-specification shall not denote an
   //   incomplete type other than a class currently being defined [...].
   //   A type denoted in an exception-specification shall not denote a
   //   pointer or reference to an incomplete type, other than (cv) void* or a
   //   pointer or reference to a class currently being defined.
-  if (!(PointeeT->isRecordType() &&
+  return !(PointeeT->isRecordType() &&
            PointeeT->getAs<RecordType>()->isBeingDefined()) &&
          RequireCompleteType(Range.getBegin(), PointeeT,
-                          diag::err_incomplete_in_exception_spec, Kind, Range))
-    return true;
-
-  return false;
+                             diag::err_incomplete_in_exception_spec, Kind,
+                             Range);
 }
 
 /// CheckDistantExceptionSpec - Check if the given type is a pointer or pointer
 /// to member to a function with an exception specification. This means that
 /// it is invalid to add another level of indirection.
 bool Sema::CheckDistantExceptionSpec(QualType T) {
   if (const PointerType *PT = T->getAs<PointerType>())
     T = PT->getPointeeType();

lib/Sema/SemaExpr.cpp

if (PointeeTy->isVoidType()) {
     diagnoseArithmeticOnVoidPointer(S, Loc, Operand);
     return !S.getLangOpts().CPlusPlus;
   }
   if (PointeeTy->isFunctionType()) {
     diagnoseArithmeticOnFunctionPointer(S, Loc, Operand);
     return !S.getLangOpts().CPlusPlus;
   }
 
-  if (checkArithmeticIncompletePointerType(S, Loc, Operand)) return false;
-
-  return true;
+  return !checkArithmeticIncompletePointerType(S, Loc, Operand);
 }
 
 /// \brief Check the validity of a binary arithmetic operation w.r.t. pointer
 /// operands.
 ///
 /// This routine will diagnose any invalid arithmetic on pointer operands much
 /// like \see checkArithmeticOpPointerOperand. However, it has special logic
 /// for emitting a single diagnostic even for operations where both LHS and RHS
static bool hasIsEqualMethod(Sema &S, const Expr *LHS, const Expr *RHS) {
   if (!Method)
     return false;
 
   QualType T = Method->parameters()[0]->getType();
   if (!T->isObjCObjectPointerType())
     return false;
 
   QualType R = Method->getReturnType();
-  if (!R->isScalarType())
-    return false;
-
-  return true;
+  return R->isScalarType();
 }
 
 Sema::ObjCLiteralKind Sema::CheckLiteralKind(Expr *FromE) {
   FromE = FromE->IgnoreParenImpCasts();
   switch (FromE->getStmtClass()) {
     default:
       break;
     case Stmt::ObjCStringLiteralClass:
void diagnose(Sema &S, SourceLocation Loc, QualType T) override {
       }
       
       S.Diag(Loc, diag::err_call_function_incomplete_return)
         << CE->getSourceRange() << FD->getDeclName() << T;
       S.Diag(FD->getLocation(), diag::note_entity_declared_at)
           << FD->getDeclName();
     }
   } Diagnoser(FD, CE);
-  
-  if (RequireCompleteType(Loc, ReturnType, Diagnoser))
-    return true;
-
-  return false;
+  return RequireCompleteType(Loc, ReturnType, Diagnoser);
 }
 
 // Diagnose the s/=/==/ and s/\|=/!=/ typos. Note that adding parentheses
 // will prevent this condition from triggering, which is what we want.
 void Sema::DiagnoseAssignmentAsCondition(Expr *E) {
   SourceLocation Loc;
 
   unsigned diagnostic = diag::warn_condition_is_assignment;

lib/Sema/SemaExprCXX.cpp

if (Matches.size() == 1) {
     if (Operator->isDeleted()) {
       if (Diagnose) {
         Diag(StartLoc, diag::err_deleted_function_use);
         NoteDeletedFunction(Operator);
       }
       return true;
     }
 
-    if (CheckAllocationAccess(StartLoc, SourceRange(), Found.getNamingClass(),
-                              Matches[0], Diagnose) == AR_inaccessible)
-      return true;
-
-    return false;
+    return CheckAllocationAccess(StartLoc, SourceRange(),
+                                 Found.getNamingClass(), Matches[0],
+                                 Diagnose) == AR_inaccessible;
 
   // We found multiple suitable operators;  complain about the ambiguity.
   } else if (!Matches.empty()) {
     if (Diagnose) {
       Diag(StartLoc, diag::err_ambiguous_suitable_delete_member_function_found)
         << Name << RD;
 
       for (SmallVectorImpl<DeclAccessPair>::iterator

lib/Sema/SemaFixItUtils.cpp

From = S.Context.getCanonicalType(
         (cast<PointerType>(From))->getPointeeType());
     To = S.Context.getCanonicalType(
         (cast<PointerType>(To))->getPointeeType());
   }
 
   const CanQualType FromUnq = From.getUnqualifiedType();
   const CanQualType ToUnq = To.getUnqualifiedType();
 
-  if ((FromUnq == ToUnq || (S.IsDerivedFrom(FromUnq, ToUnq)) ) &&
-      To.isAtLeastAsQualifiedAs(From))
-    return true;
-  return false;
+  return (FromUnq == ToUnq || (S.IsDerivedFrom(FromUnq, ToUnq))) &&
+         To.isAtLeastAsQualifiedAs(From);
 }
 
 bool ConversionFixItGenerator::tryToFixConversion(const Expr *FullExpr,
                                                   const QualType FromTy,
                                                   const QualType ToTy,
                                                   Sema &S) {
   if (!FullExpr)
     return false;

lib/Sema/SemaOpenMP.cpp

bool VisitDeclRefExpr(DeclRefExpr *E) {
     if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl())) {
       DSAStackTy::DSAVarData DVar = Stack->getTopDSA(VD, false);
       if (DVar.CKind == OMPC_shared && !DVar.RefExpr)
         return false;
       if (DVar.CKind != OMPC_unknown)
         return true;
       DSAStackTy::DSAVarData DVarPrivate =
           Stack->hasDSA(VD, isOpenMPPrivate, MatchesAlways(), false);
-      if (DVarPrivate.CKind != OMPC_unknown)
-        return true;
-      return false;
+      return DVarPrivate.CKind != OMPC_unknown;
     }
     return false;
   }
   bool VisitStmt(Stmt *S) {
     for (auto Child : S->children()) {
       if (Child && Visit(Child))
         return true;
     }

lib/Sema/SemaOverload.cpp

 /// a conversion of a pointer or pointer-to-member to bool. This is
 /// used as part of the ranking of standard conversion sequences
 /// (C++ 13.3.3.2p4).
 bool StandardConversionSequence::isPointerConversionToBool() const {
   // Note that FromType has not necessarily been transformed by the
   // array-to-pointer or function-to-pointer implicit conversions, so
   // check for their presence as well as checking whether FromType is
   // a pointer.
-  if (getToType(1)->isBooleanType() &&
+  return getToType(1)->isBooleanType() &&
          (getFromType()->isPointerType() ||
           getFromType()->isObjCObjectPointerType() ||
           getFromType()->isBlockPointerType() ||
-       getFromType()->isNullPtrType() ||
-       First == ICK_Array_To_Pointer || First == ICK_Function_To_Pointer))
-    return true;
-
-  return false;
+          getFromType()->isNullPtrType() || First == ICK_Array_To_Pointer ||
+          First == ICK_Function_To_Pointer);
 }
 
 /// isPointerConversionToVoidPointer - Determines whether this
 /// conversion is a conversion of a pointer to a void pointer. This is
 /// used as part of the ranking of standard conversion sequences (C++
 /// 13.3.3.2p4).
 bool
 StandardConversionSequence::
if (FieldDecl *MemberDecl = From->getSourceBitField()) {
 
         return false;
       }
     }
   }
 
   // An rvalue of type bool can be converted to an rvalue of type int,
   // with false becoming zero and true becoming one (C++ 4.5p4).
-  if (FromType->isBooleanType() && To->getKind() == BuiltinType::Int) {
-    return true;
-  }
-
-  return false;
+  return FromType->isBooleanType() && To->getKind() == BuiltinType::Int;
 }
 
 /// IsFloatingPointPromotion - Determines whether the conversion from
 /// FromType to ToType is a floating point promotion (C++ 4.6). If so,
 /// returns true and sets PromotedType to the promoted type.
 bool Sema::IsFloatingPointPromotion(QualType FromType, QualType ToType) {
   if (const BuiltinType *FromBuiltin = FromType->getAs<BuiltinType>())
     if (const BuiltinType *ToBuiltin = ToType->getAs<BuiltinType>()) {
bool Sema::CheckMemberPointerConversion(Expr *From, QualType ToType,
   return false;
 }
 
 /// Determine whether the lifetime conversion between the two given
 /// qualifiers sets is nontrivial.
 static bool isNonTrivialObjCLifetimeConversion(Qualifiers FromQuals,
                                                Qualifiers ToQuals) {
   // Converting anything to const __unsafe_unretained is trivial.
-  if (ToQuals.hasConst() && 
-      ToQuals.getObjCLifetime() == Qualifiers::OCL_ExplicitNone)
-    return false;
-
-  return true;
+  return !(ToQuals.hasConst() &&
+           ToQuals.getObjCLifetime() == Qualifiers::OCL_ExplicitNone);
 }
 
 /// IsQualificationConversion - Determines whether the conversion from
 /// an rvalue of type FromType to ToType is a qualification conversion
 /// (C++ 4.4).
 ///
 /// \param ObjCLifetimeConversion Output parameter that will be set to indicate
 /// when the qualification conversion involves a change in the Objective-C