diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.h --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.h @@ -15,9 +15,9 @@ namespace tidy { namespace cppcoreguidelines { -/// Finds function definitions where arguments of the same type follow each -/// other directly, making call sites prone to calling the function with swapped -/// or badly ordered arguments. +/// Finds function definitions where arguments of the essentially similar type +/// follow each other directly, making call sites prone to calling the function +/// with swapped or badly ordered arguments. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.html @@ -36,6 +36,10 @@ /// Whether to consider 'T' and 'const T'/'volatile T'/etc. arguments to be /// possible mixup. const bool CVRMixPossible; + + /// Whether to consider implicit conversion possibilities as a potential match + /// for adjacency. + const bool ImplicitConversion; }; } // namespace cppcoreguidelines diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp @@ -10,6 +10,7 @@ #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "llvm/ADT/BitmaskEnum.h" +#include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include @@ -20,6 +21,28 @@ #define DEBUG_TYPE "AdjacentArguments" #include "llvm/Support/Debug.h" // clang-format on +template inline std::string format_bytes_do(std::size_t N) { + static_assert(std::is_unsigned::value, + "size_t is not unsigned?!"); + constexpr std::size_t WidthWithPadding = Width + (Width / 4); + char C[WidthWithPadding]; + for (std::size_t CurPos = 0; CurPos < WidthWithPadding; ++CurPos) { + if (CurPos % 5 == 0) { + // Group digits by 4. + C[CurPos] = ' '; + continue; + } + + C[CurPos] = N % 2 ? '1' : '0'; + N >>= 1; + } + + return std::string{std::rbegin(C), std::rend(C)}; +} + +template inline std::string format_bytes(T &&t) { + return format_bytes_do(t); +} using namespace clang::ast_matchers; @@ -29,64 +52,160 @@ namespace { -/// Annotates how a mixup might happen. This is a flag enumeration. +// Set the bit at index N to 1 as the enum constant. N = 0 is invalid. +#define FBIT(Name, N) MIXUP_##Name = (1ull << (N##ull - 1ull)) +/// Annotates how a mixup might happen. This is a bit flag. enum MixupTag : unsigned char { MIXUP_Invalid = 0, //< Sentinel 0 bit pattern value for masking. DO NOT USE! - MIXUP_None = 1, //< Mixup is not possible. - MIXUP_Trivial = 2, //< No extra information needed. - MIXUP_Typedef = 4, //< Argument of a typedef which resolves to an + FBIT(None, 1), //< Mixup is not possible. + FBIT(Trivial, 2), //< No extra information needed. + FBIT(Typedef, 3), //< Argument of a typedef which resolves to an //< effective desugared type same as the other arg. - MIXUP_RefBind = 8, //< Argument mixes with another due to reference binding. - MIXUP_CVR = 16, //< Argument mixes with another through implicit + FBIT(RefBind, 4), //< Argument mixes with another due to reference binding. + FBIT(CVR, 5), //< Argument mixes with another through implicit //< qualification. + FBIT(Implicit, 6), //< An implicit conversion may happen. - LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ MIXUP_CVR) + LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ MIXUP_Implicit) }; +#undef FBIT LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE(); -using AdjacentParamsMixup = - llvm::SmallVector, 4>; +/// Implicit conversion sequence steps resulting in types referred here. +struct ConversionSequence { + /// Type of the intermediate value after the first standard conversion. + const Type *StdPre; + /// Type of the intermediate value after executing the user-defined conversion + /// which, in case of constructors, the user type, in case of conversion + /// operators, the result of the operator. + const Type *UserType; + /// Type of the intermediate value after the second standard conversion. + const Type *StdPost; + + ConversionSequence() : StdPre(nullptr), UserType(nullptr), StdPost(nullptr) {} + + explicit operator bool() const { return StdPre || UserType || StdPost; } + + /// Whether the conversion sequence is single-step only. + bool single() const { + return ((bool)StdPre ^ (bool)UserType ^ (bool)StdPost) && + !(StdPre && UserType && StdPost); + } +}; -} // namespace +struct MixupData { + MixupTag Flag; + ConversionSequence ConvLTR, ConvRTL; -/// Attempts to strip away as many qualification, sugar and spice from the -/// type as possible to reach the innermost purest the user eventually uses. -/// This method attempts to discard everything as much as possible. -static const Type *stripType(const Type *T); + MixupData(MixupTag Flags) : Flag(Flags) {} + MixupData(MixupTag Flags, const ConversionSequence &Seq) + : Flag(Flags), ConvLTR(Seq), ConvRTL(Seq) {} + MixupData(MixupTag Flags, const ConversionSequence <R, + const ConversionSequence &RTL) + : Flag(Flags), ConvLTR(LTR), ConvRTL(RTL) {} -/// Returns whether an lvalue reference refers to the same type as T. -static MixupTag refBindsToSameType(const LValueReferenceType *LRef, - const Type *T, const bool CVRMixPossible); + MixupData operator|(MixupTag EnableFlag) const { + return {Flag | EnableFlag, ConvLTR, ConvRTL}; + } + MixupData &operator|=(MixupTag EnableFlag) { + Flag |= EnableFlag; + return *this; + } -/// Sanitises the MixupTag so it doesn't contain contradictory bits set. -static MixupTag sanitiseMixup(MixupTag Tag) { - assert(Tag != MIXUP_Invalid && "Mixup tag had full zero bit pattern value!"); + /// Sanitises the mixup so it doesn't contain contradictory bits set. + void sanitise() { + assert(Flag != MIXUP_Invalid && + "Mixup tag had full zero bit pattern value!"); - if (Tag & MIXUP_None) - // If at any point the check recursion marks the mixup impossible, it is - // just impossible. - return MIXUP_None; + if (Flag & MIXUP_None) { + // If at any point the check recursion marks the mixup impossible, it is + // just impossible. + Flag = MIXUP_None; + return; + } - if (Tag == MIXUP_Trivial) - return Tag; + if (Flag == MIXUP_Trivial) + return; - if (Tag ^ MIXUP_Trivial) - // If any other bits than Trivial is set, unset Trivial, so only the - // annotation bits warranting extra diagnostic are set. - return Tag & ~MIXUP_Trivial; + if (Flag ^ MIXUP_Trivial) + // If any other bits than Trivial is set, unset Trivial, so only the + // annotation bits warranting extra diagnostic are set. + Flag &= ~MIXUP_Trivial; - return Tag; -} + // Set LTR and RTL implicity according to the members being set. + if (ConvLTR || ConvRTL) + Flag |= MIXUP_Implicit; + else + Flag &= ~MIXUP_Implicit; + } +}; + +/// Named tuple that contains that the types of the arguments From and To +/// are mixable with the given flags in a particular fashion. +struct Mixup { + const ParmVarDecl *From, *To; + MixupData Data; +}; +static_assert(std::is_trivially_copyable::value, + "keep Mixup and components trivially copyable!"); + +struct AdjacentParamsMixup { + std::size_t NumParamsChecked = 1; + llvm::SmallVector Mixups; + + const ParmVarDecl *getFirstParm() const { + // The first element added to the mixup vector has the left hand + // side set to the first argument in the range, if any is added. + assert(!Mixups.empty()); + return Mixups.front().From; + } -/// Returns whether LType and RType refer to the same type in a sense that at a -/// call site it is possible to mix the types up if the actual arguments are -/// specified in opposite order. -/// \returns MixupTag indicating how a mixup between the arguments happens. -/// The final output of this potentially recursive function must be sanitised. -static MixupTag howPossibleToMixUpAtCallSite(const QualType LType, - const QualType RType, - const ASTContext &Ctx, - const bool CVRMixPossible) { + const ParmVarDecl *getLastParm() const { + // There builder function paramEqualTypeRange breaks building instance of + // this type if it finds something that can not be mixed up, by going + // *forward* in the list of arguments. So at the moment of break, the right + // hand side of the last conversion in the vector is the last argument in + // the adjacency range. + assert(!Mixups.empty()); + return Mixups.back().To; + } +}; + +} // namespace + +/// Attempts to strip away as many qualification, sugar and spice from the +/// type as possible to reach the innermost purest the user eventually uses. +/// This method attempts to discard everything as much as possible. +static const Type *stripType(const Type *T); + +/// Returns how an lvalue reference refers to the same type as T. +static MixupData refBindsToSameType(const LValueReferenceType *LRef, + const Type *T, const ASTContext &Ctx, + const bool IsRefRightType, + const bool CVRMixPossible, + const bool ImplicitConversion); + +/// Returns whether the left side type is convertible to the right side type, +/// by attempting to approximate implicit conversion sequences. +/// \param AllowUserDefined If false, only standard conversions will be +/// approximated. +/// \note The result of this operation is not symmetric! +static MixupData howConvertible(const Type *LT, const Type *RT, + const LangOptions &LOpts, + bool AllowUserDefined = true); + +/// Returns how LType and RType may essentially refer to the same type - in a +/// sense that at a call site it is possible to mix the arguments up if +/// specified in the opposite order. +/// \returns MixupData indicating how a mixup between the arguments happens. +/// \note The final output of this potentially recursive function must be +/// sanitised by 'sanitiseMixup' before it could be used, to ensure only the +/// proper bits are set! +static MixupData howPossibleToMixUpAtCallSite(const QualType LType, + const QualType RType, + const ASTContext &Ctx, + const bool CVRMixPossible, + const bool ImplicitConversion) { LLVM_DEBUG(llvm::dbgs() << "isPossibleToMixUpAtCallSite?\n Left:"; LType.dump(llvm::dbgs()); llvm::dbgs() << "\n\t and \n Right: "; RType.dump(llvm::dbgs()); llvm::dbgs() << '\n';); @@ -97,12 +216,69 @@ } // Remove certain sugars that don't affect mixability from the types. - if (dyn_cast(LType.getTypePtr())) + if (dyn_cast(LType.getTypePtr())) { + LLVM_DEBUG(llvm::dbgs() << "Left is some sugared Parens()\n"); return howPossibleToMixUpAtCallSite(LType.getSingleStepDesugaredType(Ctx), - RType, Ctx, CVRMixPossible); - if (dyn_cast(RType.getTypePtr())) + RType, Ctx, CVRMixPossible, + ImplicitConversion); + } + if (dyn_cast(RType.getTypePtr())) { + LLVM_DEBUG(llvm::dbgs() << "Right is some sugared Parens()\n"); return howPossibleToMixUpAtCallSite( - LType, RType.getSingleStepDesugaredType(Ctx), Ctx, CVRMixPossible); + LType, RType.getSingleStepDesugaredType(Ctx), Ctx, CVRMixPossible, + ImplicitConversion); + } + + // An argument of type 'T' and 'const T &' may bind with the same power. + // (Note this is a different case, as 'const T &' is a '&' on the top level, + // and only then a const.) + if (LType->isLValueReferenceType() || RType->isLValueReferenceType()) { + // (If both is the same reference type, earlier a return happened.) + + if (LType->isLValueReferenceType()) { + LLVM_DEBUG(llvm::dbgs() << "!!! Left is an lvalue reference type\n";); + // Return value of function call may or may not have given us a tag (e.g. + // reference was to a typedef) via a recursive chain back to this + // function. Apply the "bind power" tag here to indicate a reference + // binding happened. (If RefBind was MIXUP_None, a later sanitise step + // will undo every bit except for None.) + return refBindsToSameType(LType->getAs(), + RType.getTypePtr(), Ctx, + /* IsRefRightType =*/false, CVRMixPossible, + ImplicitConversion) | + MIXUP_RefBind; + } + if (RType->isLValueReferenceType()) { + LLVM_DEBUG(llvm::dbgs() << "!!! Right is an lvalue reference type.\n";); + return refBindsToSameType(RType->getAs(), + LType.getTypePtr(), Ctx, + /* IsRefRightType =*/true, CVRMixPossible, + ImplicitConversion) | + MIXUP_RefBind; + } + } + + // An argument of type 'T' and 'const T' may bind with the same power. + // Case for both types being const qualified (for the same type) is handled + // by LType == RType. + if (LType.getLocalCVRQualifiers() || RType.getLocalCVRQualifiers()) { + LLVM_DEBUG( + llvm::dbgs() + << "!!! Left or right is locally CVR: Left: " + << format_bytes( + (unsigned char)LType.getLocalQualifiers().getCVRQualifiers()) + << ", Right: " + << format_bytes( + (unsigned char)RType.getLocalQualifiers().getCVRQualifiers()) + << '\n'); + if (!CVRMixPossible) + return MIXUP_None; + + return howPossibleToMixUpAtCallSite(LType.getUnqualifiedType(), + RType.getUnqualifiedType(), Ctx, + CVRMixPossible, ImplicitConversion) | + MIXUP_CVR; + } { const auto *LTypedef = LType->getAs(); @@ -110,94 +286,87 @@ if (LTypedef && RTypedef) { LLVM_DEBUG(llvm::dbgs() << "!!! Both Left and Right are typedef types.\n";); - return MIXUP_Typedef | howPossibleToMixUpAtCallSite(LTypedef->desugar(), - RTypedef->desugar(), - Ctx, CVRMixPossible); + return howPossibleToMixUpAtCallSite(LTypedef->desugar(), + RTypedef->desugar(), Ctx, + CVRMixPossible, ImplicitConversion) | + MIXUP_Typedef; } if (LTypedef) { LLVM_DEBUG(llvm::dbgs() << "!!! Left is typedef type.\n";); - return MIXUP_Typedef | - howPossibleToMixUpAtCallSite(LTypedef->desugar(), RType, Ctx, - CVRMixPossible); + return howPossibleToMixUpAtCallSite(LTypedef->desugar(), RType, Ctx, + CVRMixPossible, ImplicitConversion) | + MIXUP_Typedef; } if (RTypedef) { LLVM_DEBUG(llvm::dbgs() << "!!! Right is typedef type.\n";); - return MIXUP_Typedef | - howPossibleToMixUpAtCallSite(LType, RTypedef->desugar(), Ctx, - CVRMixPossible); + return howPossibleToMixUpAtCallSite(LType, RTypedef->desugar(), Ctx, + CVRMixPossible, ImplicitConversion) | + MIXUP_Typedef; } } if (LType->isPointerType() && RType->isPointerType()) { // (Both types being the exact same pointer is handled by LType == RType.) + // The implicit conversion between any T* and U* possible in C is ignored, + // as virtually all compilers emit a warning if the conversion is done at a + // call site. LLVM_DEBUG(llvm::dbgs() << "!!! Both Left and Right are pointer types.\n";); - MixupTag Mixup = howPossibleToMixUpAtCallSite( - LType->getPointeeType(), RType->getPointeeType(), Ctx, CVRMixPossible); - - if (LType.isLocalConstQualified() || LType.isLocalVolatileQualified() || - LType.isLocalRestrictQualified() || RType.isLocalConstQualified() || - RType.isLocalVolatileQualified() || RType.isLocalRestrictQualified()) { - // A qualified ptr might be mixed up with an unqualified one at call, but - // this is up to user configuration to report about. - if (CVRMixPossible) - Mixup |= MIXUP_CVR; - else - Mixup |= MIXUP_None; - } - return Mixup; - } + // Implicit conversions of two pointers should be diagnosed if they point to + // C++ records to find Derived-To-Base implicit casts. + // For non-user-types, do not check - giving an unrelated, e.g. "long *" in + // place of an "int *" is diagnosed anyways by warnings or errors. + bool ShouldDiagnoseImplicitBehindPtr = ImplicitConversion && + LType->getPointeeCXXRecordDecl() && + RType->getPointeeCXXRecordDecl(); + LLVM_DEBUG( + llvm::dbgs() + << "Checking mixup between pointed types. Allowing implicit modelling? " + << ShouldDiagnoseImplicitBehindPtr << "\nOriginally the user specified " + << ImplicitConversion << ".\n"); - // An argument of type 'T' and 'const T' may bind with the same power. - // Case for both types being const qualified (for the same type) is handled - // by LType == RType. - if (CVRMixPossible && - (LType.isLocalConstQualified() || LType.isLocalVolatileQualified())) { - LLVM_DEBUG(llvm::dbgs() << "!!! Left is CV qualified."; - LType.dump(llvm::dbgs()); llvm::dbgs() << '\n';); - return MIXUP_CVR | howPossibleToMixUpAtCallSite(LType.getUnqualifiedType(), - RType, Ctx, CVRMixPossible); - } - if (CVRMixPossible && - (RType.isLocalConstQualified() || RType.isLocalVolatileQualified())) { - LLVM_DEBUG(llvm::dbgs() << "!!! Right is CV qualified\n"; - RType.dump(llvm::dbgs()); llvm::dbgs() << '\n';); - return MIXUP_CVR | - howPossibleToMixUpAtCallSite(LType, RType.getUnqualifiedType(), Ctx, - CVRMixPossible); + return howPossibleToMixUpAtCallSite( + LType->getPointeeType(), RType->getPointeeType(), Ctx, CVRMixPossible, + ShouldDiagnoseImplicitBehindPtr); } - // An argument of type 'T' and 'const T &' may bind with the same power. - // (Note this is a different case, as 'const T &' is a '&' on the top level, - // and only then a const.) - if (LType->isLValueReferenceType() || RType->isLValueReferenceType()) { - // (If both is the same reference type, earlier a return happened.) - - if (LType->isLValueReferenceType()) { - LLVM_DEBUG(llvm::dbgs() << "!!! Left is an lvalue reference type\n";); - MixupTag RefBind = refBindsToSameType(LType->getAs(), - RType.getTypePtr(), CVRMixPossible); - // RefBind may or may not have given us a tag (e.g. reference was to a - // typedef) via a recursive chain back to this function. Apply the - // "bind power" tag here to indicate a reference binding happened. - // (If RefBind was MIXUP_None, a later sanitise step will undo every bit - // except for None.) - return RefBind | MIXUP_RefBind; - } - if (RType->isLValueReferenceType()) { - LLVM_DEBUG(llvm::dbgs() << "!!! Right is an lvalue reference type.\n";); - MixupTag RefBind = refBindsToSameType(RType->getAs(), - LType.getTypePtr(), CVRMixPossible); - return RefBind | MIXUP_RefBind; - } + if (ImplicitConversion) { + LLVM_DEBUG(llvm::dbgs() + << "Implicit conversion check has been enabled! Checking...\n"); + + const Type *LT = LType.getTypePtr(); + const Type *RT = RType.getTypePtr(); + + // Try approximating an implicit conversion sequence. + LLVM_DEBUG(llvm::dbgs() << "?????????? Check Left->Right ..............\n"); + MixupData LTR = + howConvertible(LT, RT, Ctx.getLangOpts(), /* AllowUserDefined =*/true); + LLVM_DEBUG(llvm::dbgs() + << "Left->Right flags: " << format_bytes(LTR.Flag) << '\n'); + if (LTR.Flag != MIXUP_None) + LLVM_DEBUG(llvm::dbgs() << "Left -> Right mixup\n"); + + LLVM_DEBUG(llvm::dbgs() << "?????????? Check Right->Left ..............\n"); + MixupData RTL = + howConvertible(RT, LT, Ctx.getLangOpts(), /* AllowUserDefined =*/true); + LLVM_DEBUG(llvm::dbgs() + << "Left<-Right flags: " << format_bytes(RTL.Flag) << '\n'); + if (RTL.Flag != MIXUP_None) + LLVM_DEBUG(llvm::dbgs() << "Left <- Right mixup\n"); + + if (LTR.ConvLTR || RTL.ConvRTL) + return {MIXUP_Implicit, LTR.ConvLTR, RTL.ConvRTL}; } - LLVM_DEBUG(llvm::dbgs() << "<<< End of logic reached, types don't match.";); + LLVM_DEBUG(llvm::dbgs() << "<<< End of logic reached, types don't match.\n";); return MIXUP_None; } -static MixupTag refBindsToSameType(const LValueReferenceType *LRef, - const Type *T, const bool CVRMixPossible) { +static MixupData refBindsToSameType(const LValueReferenceType *LRef, + const Type *T, const ASTContext &Ctx, + const bool IsRefRightType, + const bool CVRMixPossible, + const bool ImplicitConversion) { LLVM_DEBUG(llvm::dbgs() << "refBindsToSameType?\n"; LRef->dump(llvm::dbgs()); llvm::dbgs() << "\n\t and \n"; T->dump(llvm::dbgs()); llvm::dbgs() << '\n';); @@ -212,18 +381,379 @@ return MIXUP_None; } - if (const auto *TypedefTy = ReferredType.getTypePtr()->getAs()) { + if (const auto *TypedefTy = ReferredType->getAs()) { // If the referred type is a typedef, try checking the mixup-chance on the // desugared type. LLVM_DEBUG(llvm::dbgs() << "Reference to a typedef, calling back...\n";); - return howPossibleToMixUpAtCallSite(TypedefTy->desugar(), QualType{T, 0}, - TypedefTy->getDecl()->getASTContext(), - CVRMixPossible); + if (!IsRefRightType) + return howPossibleToMixUpAtCallSite(TypedefTy->desugar(), QualType{T, 0}, + Ctx, CVRMixPossible, + ImplicitConversion); + return howPossibleToMixUpAtCallSite(QualType{T, 0}, TypedefTy->desugar(), + Ctx, CVRMixPossible, + ImplicitConversion); } LLVM_DEBUG(llvm::dbgs() << "is referred type type_ptr() and T the same? " << (ReferredType.getTypePtr() == T) << '\n';); - return ReferredType.getTypePtr() == T ? MIXUP_Trivial : MIXUP_None; + if (ReferredType.getTypePtr() == T) + return MIXUP_Trivial; + if (ImplicitConversion) { + // Try to see if the reference can be bound through an implicit conversion. + if (!IsRefRightType) + return howPossibleToMixUpAtCallSite(ReferredType.getUnqualifiedType(), + QualType{T, 0}, Ctx, CVRMixPossible, + ImplicitConversion); + return howPossibleToMixUpAtCallSite(QualType{T, 0}, + ReferredType.getUnqualifiedType(), Ctx, + CVRMixPossible, ImplicitConversion); + } + return MIXUP_None; +} + +static inline bool isNumericConvertible(const Type *LT, const Type *RT) { + LLVM_DEBUG(llvm::dbgs() << "isNumericConvertible(" << LT << ", " << RT + << ")\n"); + if (!LT || !RT) + return false; + + bool LI = LT->isIntegerType(); + bool LF = LT->isFloatingType(); + bool RI = RT->isIntegerType(); + bool RF = RT->isFloatingType(); + + // Through promotions and conversions, there is free passage between + // "integer types" and "floating types". + // Promotions don't change value, conversions may result in value or precision + // loss, but for the sake of easy understanding, we put the two under + // one umbrella. + return (LI && RI) || (LF && RF) || (LI && RF) || (LF && RI); +} + +/// Returns a Mixup indicating if the specified converting constructor can be +/// called with LT or after applying a standard conversion sequence on LT. +static MixupData tryConvertingConstructor(const Type *LT, + const CXXRecordDecl *RD, + const CXXConstructorDecl *Conv, + const LangOptions &LOpts) { + const ParmVarDecl *ConArg = Conv->getParamDecl(0); + MixupData Mix = howPossibleToMixUpAtCallSite( + QualType{LT, 0}, ConArg->getType(), Conv->getASTContext(), + // Model qualifier mixing generously. + /* CVRMixPossible =*/true, + // Do not allow any further implicit conversions. + /* ImplicitConversion =*/false); + LLVM_DEBUG(llvm::dbgs() << "Conv no-std (pre sanitise): " + << format_bytes(Mix.Flag) << '\n'); + Mix.sanitise(); + LLVM_DEBUG(llvm::dbgs() << "Conv no-std (post sanitise): " + << format_bytes(Mix.Flag) << '\n'); + if (Mix.Flag != MIXUP_None) { + // If simply applying the user-defined constructor works as is, this is + // the right constructor. + LLVM_DEBUG(llvm::dbgs() << "Conversion constructor matches exactly.\n"); + ConversionSequence Seq; + Seq.UserType = RD->getTypeForDecl(); + Mix.ConvLTR = Seq; + return Mix; + } + + // Otherwise, see if LT and the converting constructor's argument can be + // matched via a standard conversion before. This will take care of converting + // an int to another type. + ConversionSequence Seq; + MixupData Pre = howConvertible(LT, ConArg->getType().getTypePtr(), LOpts, + /* AllowUserDefined =*/false); + LLVM_DEBUG(llvm::dbgs() << "Pre-UD: " << format_bytes(Pre.Flag) + << " to type \n"; + Pre.ConvLTR.StdPre->dump(llvm::dbgs()); llvm::dbgs() << '\n'); + if (Pre.Flag == MIXUP_None || Pre.Flag == MIXUP_Trivial) + // If not possible to mix, don't check. If trivial, previous case should + // have returned already. + return MIXUP_None; + Seq.StdPre = Pre.ConvLTR.StdPre; + + LLVM_DEBUG(llvm::dbgs() + << "Conversion constructor matches after standard conversion.\n"); + + Mix = howPossibleToMixUpAtCallSite( + QualType{Seq.StdPre, 0}, ConArg->getType(), Conv->getASTContext(), + // Model qualifier mixing generously. + /* CVRMixPossible =*/true, + // Do not allow any further implicit conversions. + /* ImplicitConversion =*/false); + LLVM_DEBUG(llvm::dbgs() << "Conv (pre sanitise): " << format_bytes(Mix.Flag) + << '\n'); + Mix.sanitise(); + LLVM_DEBUG(llvm::dbgs() << "Conv (post sanitise): " << format_bytes(Mix.Flag) + << '\n'); + if (Mix.Flag != MIXUP_None) { + LLVM_DEBUG(llvm::dbgs() << "Conversion constructor matches!\n"); + Seq.UserType = RD->getTypeForDecl(); + Mix.ConvLTR = Seq; + return Mix; + } + + return MIXUP_None; +} + +static MixupData tryConversionOperator(const CXXConversionDecl *Conv, + const Type *RT, + const LangOptions &LOpts) { + MixupData Mix = howPossibleToMixUpAtCallSite( + Conv->getConversionType(), QualType{RT, 0}, Conv->getASTContext(), + // Model qualifier mixing generously. + /* CVRMixPossible =*/true, + // Do not allow any further implicit conversions. + /* ImplicitConversion =*/false); + LLVM_DEBUG(llvm::dbgs() << "Conv no-std (pre sanitise): " + << format_bytes(Mix.Flag) << '\n'); + Mix.sanitise(); + LLVM_DEBUG(llvm::dbgs() << "Conv no-std (post sanitise): " + << format_bytes(Mix.Flag) << '\n'); + if (Mix.Flag != MIXUP_None) { + // If simply applying the user-defined operator works as is, this is + // the right conversion operator. + ConversionSequence Seq; + Seq.UserType = RT; + Mix.ConvLTR = Seq; + return Mix; + } + + // Otherwise, see if the converting operator's result and RT can be + // matched via a standard conversion after. This will take care of converting + // a result of float to another type. + ConversionSequence Seq; + MixupData Post = howConvertible(Conv->getConversionType().getTypePtr(), RT, + LOpts, /* AllowUserDefined =*/false); + LLVM_DEBUG(llvm::dbgs() << "Post-UD: " << format_bytes(Post.Flag) + << " to type \n"; + Post.ConvLTR.StdPre->dump(llvm::dbgs()); llvm::dbgs() << '\n'); + if (Post.Flag == MIXUP_None || Post.Flag == MIXUP_Trivial) + // If not possible to mix, don't check. If trivial, previous case should + // have returned already. + return MIXUP_None; + // Save the result type of the user-defined conversion operator. + Seq.UserType = Conv->getConversionType().getTypePtr(); + + // The unqualified version of the conversion operator's return type can be + // converted to the "needed" Right-Type (output type wanted by the caller + // of this method). Now see if between we lost any qualifiers. + Mix = howPossibleToMixUpAtCallSite( + Conv->getConversionType(), + QualType{Conv->getConversionType().getTypePtr(), 0}, + Conv->getASTContext(), + // Model qualifier mixing generously. + /* CVRMixPossible =*/true, + // Do not allow any further implicit conversions. + /* ImplicitConversion =*/false); + LLVM_DEBUG(llvm::dbgs() << "Conv (pre sanitise): " << format_bytes(Mix.Flag) + << '\n'); + Mix.sanitise(); + LLVM_DEBUG(llvm::dbgs() << "Conv (post sanitise): " << format_bytes(Mix.Flag) + << '\n'); + if (Mix.Flag != MIXUP_None) { + LLVM_DEBUG(llvm::dbgs() << "Conversion operator matches!\n"); + Seq.StdPost = RT; + Mix.ConvLTR = Seq; + return Mix; + } + + return MIXUP_None; +} + +static MixupData howConvertible(const Type *LT, const Type *RT, + const LangOptions &LOpts, + bool AllowUserDefined) { + LLVM_DEBUG(llvm::dbgs() << ">>>>> how convertible? \n"; + LT->dump(llvm::dbgs()); llvm::dbgs() << '\n'; + RT->dump(llvm::dbgs()); llvm::dbgs() << '\n'); + if (LT == RT) { + LLVM_DEBUG(llvm::dbgs() + << "Left and right is the same, returning Trivial.\n"); + return MIXUP_Trivial; + } + + ConversionSequence Seq; + + const auto *LBT = LT->getAs(); + const auto *RBT = RT->getAs(); + if (LBT && RBT && LBT == RBT) + return MIXUP_Trivial; + if (isNumericConvertible(LBT, RBT)) { + // Builtin numerical types are back-and-forth convertible. + LLVM_DEBUG(llvm::dbgs() << "Builtin conversion between numerics.\n"); + Seq.StdPre = RBT; + return {MIXUP_Implicit, Seq}; + } + + const auto *LET = LT->getAs(); + const auto *RET = RT->getAs(); + if (LET && !LET->isScopedEnumeralType() && RBT && + (RBT->isIntegerType() || RBT->isFloatingType())) { + // Enum can convert to underlying integer type. + LLVM_DEBUG(llvm::dbgs() << "Enum -> integer/float.\n"); + Seq.StdPre = RBT; + return {MIXUP_Implicit, Seq}; + } + if (LBT && (LBT->isFloatingType() || LBT->isIntegerType()) && RET) { + // Enum cannot be constructed from any builtin type (neither int, nor + // float). + if (LOpts.CPlusPlus) { + LLVM_DEBUG(llvm::dbgs() << "Numeric -/-> Enum NOT POSSIBLE (C++).\n"); + return MIXUP_None; + } + // In C, you can go back and forth between numeric types. + Seq.StdPre = RET; + return {MIXUP_Implicit, Seq}; + } + + if (LOpts.CPlusPlus) { + // We are checking Left -> Right mixup, in which case Left <: Right should + // hold for DerivedToBase implicit cast. This is a standard implicit + // conversion. + const auto *LCXXRec = LT->getAsCXXRecordDecl(); + const auto *RCXXRec = RT->getAsCXXRecordDecl(); + LLVM_DEBUG(llvm::dbgs() << "Left as record: " << LCXXRec + << ", Right as record: " << RCXXRec << '\n'); + if (LCXXRec && RCXXRec && LCXXRec->isCompleteDefinition() && + RCXXRec->isCompleteDefinition() && + (LCXXRec->isDerivedFrom(RCXXRec) || + LCXXRec->isVirtuallyDerivedFrom(RCXXRec))) { + LLVM_DEBUG(llvm::dbgs() << "Left <: Right, giving standard DerivedToBase " + "cast Left->Right.\n"); + Seq.StdPre = RT; + return {MIXUP_Implicit, Seq}; + } + } + + LLVM_DEBUG(llvm::dbgs() << "Before checking user defined...\n"); + + if (!LOpts.CPlusPlus || !AllowUserDefined) + // User-defined conversions are only sensible in C++ mode. + return MIXUP_None; + assert(!Seq && "Non-user defined conversion check should've returned."); + + LLVM_DEBUG(llvm::dbgs() << "Checking for user-defined conversions...\n"); + MixupTag UserMixup = MIXUP_Invalid; + bool FoundConvertingCtor = false, FoundConversionOperator = false; + bool FoundMultipleMatches = false; + // An implicit conversion sequence may only contain at most *one* user-defined + // conversion. + if (const auto *RRT = RT->getAs()) { + LLVM_DEBUG(llvm::dbgs() << "Right hand side is a record type.\n"); + const auto *RD = RRT->getAsCXXRecordDecl(); + if (!RD) + // Initialisation of C-style record types from an unrelated type is not + // possible. + return MIXUP_None; + if (!RD->isCompleteDefinition()) + // Incomplete definition means we don't know anything about members. + return MIXUP_None; + + for (const CXXConstructorDecl *Con : RD->ctors()) { + if (Con->isCopyOrMoveConstructor() || + !Con->isConvertingConstructor(/* AllowExplicit =*/false)) + continue; + LLVM_DEBUG(llvm::dbgs() << "Found a converting constructor? \n"; + Con->dump(llvm::dbgs()); llvm::dbgs() << '\n'); + + MixupData ConvertingResult = tryConvertingConstructor(LT, RD, Con, LOpts); + if (ConvertingResult.Flag == MIXUP_None) + continue; + + LLVM_DEBUG(llvm::dbgs() << "Came back with a positive result.\n"); + FoundConvertingCtor = true; + + if (!ConvertingResult.ConvLTR.StdPre) { + // A match without a pre-conversion was found for the converting ctor. + LLVM_DEBUG(llvm::dbgs() << "! EXACT MATCH.\n"); + UserMixup |= ConvertingResult.Flag; + Seq = ConvertingResult.ConvLTR; + FoundMultipleMatches = false; + break; + } + + if (!FoundMultipleMatches && Seq) { + // If the loop executes multiple times, it's possible to find multiple + // converting constructors with different, ambiguous conversion + // sequences leading up to them. This is an error, but we need to check + // all constructors first for a possible exact match. + FoundMultipleMatches = true; + continue; + } + + // Register the match. + UserMixup |= ConvertingResult.Flag; + Seq = ConvertingResult.ConvLTR; + } + } + + if (const auto *LRT = LT->getAs()) { + LLVM_DEBUG(llvm::dbgs() << "Left hand side is a record type.\n"); + const auto *RD = LRT->getAsCXXRecordDecl(); + if (!RD) + // Initialisation of things from an unrelated C-style record type is not + // possible. + return MIXUP_None; + if (!RD->isCompleteDefinition()) + // Against getVisibleConversionFunctions() assert. + return MIXUP_None; + + for (const NamedDecl *Method : RD->getVisibleConversionFunctions()) { + LLVM_DEBUG(llvm::dbgs() << "Found visible conv operator: \n"; + Method->dump(llvm::dbgs()); llvm::dbgs() << '\n';); + + const auto *Con = dyn_cast(Method); + if (!Con || Con->isExplicit()) + continue; + LLVM_DEBUG(llvm::dbgs() << "Found a conversion method? \n"; + Con->dump(llvm::dbgs()); llvm::dbgs() << '\n'); + + MixupData ConvertingResult = tryConversionOperator(Con, RT, LOpts); + if (ConvertingResult.Flag == MIXUP_None) + continue; + + LLVM_DEBUG(llvm::dbgs() << "Came back with a positive result.\n"); + FoundConversionOperator = true; + + if (!ConvertingResult.ConvLTR.StdPost) { + // A match without a pre-conversion was found for the converting oper. + LLVM_DEBUG(llvm::dbgs() << "! EXACT MATCH.\n"); + UserMixup |= ConvertingResult.Flag; + Seq = ConvertingResult.ConvLTR; + FoundMultipleMatches = false; + break; + } + + if (!FoundMultipleMatches && Seq) { + // If the loop executes multiple times, it's possible to find multiple + // conversion operators with different, ambiguous conversion + // sequences coming from them. This is an error, but we need to check + // all constructors first for a possible exact match. + FoundMultipleMatches = true; + continue; + } + + // Register the match. + UserMixup |= ConvertingResult.Flag; + Seq = ConvertingResult.ConvLTR; + } + } + + if (FoundMultipleMatches) { + LLVM_DEBUG(llvm::dbgs() << "Multiple conversion sequences found! :(\n"); + return MIXUP_None; + } + if (FoundConvertingCtor && FoundConversionOperator) { + LLVM_DEBUG(llvm::dbgs() << "Left<->Right ambiguous as both can be " + "converted to each other...\n"); + return MIXUP_None; + } + + LLVM_DEBUG(llvm::dbgs() << "Flags at the end of 'howConvertible?': " + << format_bytes(UserMixup) << '\n'); + return Seq ? MixupData{UserMixup, Seq} : MixupData{MIXUP_None}; } /// Gets the type-equal range of the parameters of F starting with the param @@ -233,46 +763,77 @@ /// reasoning behind mixing up the nth parameter with the first in the adjacent /// range. static AdjacentParamsMixup paramEqualTypeRange(const FunctionDecl *F, - unsigned int i, - const bool CVRMixPossible) { - assert(i <= F->getNumParams() && "invalid index given"); - + unsigned int StartIdx, + const bool CVRMixPossible, + const bool ImplicitConversion) { AdjacentParamsMixup APR; - const ParmVarDecl *First = F->getParamDecl(i); - LLVM_DEBUG(llvm::dbgs() << "Start " << i << "th param:\n"; - First->dump(llvm::dbgs()); llvm::dbgs() << '\n';); - APR.emplace_back(First, MIXUP_Trivial); - const ASTContext &Ctx = F->getASTContext(); const unsigned int ParamCount = F->getNumParams(); + assert(StartIdx < ParamCount && "invalid start index given!"); + const ASTContext &Ctx = F->getASTContext(); + + const auto CalcMixupBetween = [&Ctx, CVRMixPossible, + ImplicitConversion](const ParmVarDecl *A, + const ParmVarDecl *B) { + MixupData MD = howPossibleToMixUpAtCallSite( + A->getType(), B->getType(), Ctx, CVRMixPossible, ImplicitConversion); + LLVM_DEBUG(llvm::dbgs() << "\tMixup (before sanitise)? " + << format_bytes(MD.Flag) << "\n";); + MD.sanitise(); + LLVM_DEBUG(llvm::dbgs() << "\tMixup (after sanitise)? " + << format_bytes(MD.Flag) << "\n";); + assert(MD.Flag != MIXUP_Invalid && + "Bits fell off, result is sentinel value."); + + return Mixup{A, B, MD}; + }; + + const ParmVarDecl *First = F->getParamDecl(StartIdx); + LLVM_DEBUG(llvm::dbgs() << "Start at " << StartIdx << "th param:\n"; + First->dump(llvm::dbgs()); llvm::dbgs() << '\n';); + // FIXME: Make this method get the minimum length required and do not build // ranges that are shorter than requirement - they'd be discarded anyways. - for (++i; i < ParamCount; ++i) { + for (unsigned int i = StartIdx + 1; i < ParamCount; ++i) { const ParmVarDecl *Ith = F->getParamDecl(i); LLVM_DEBUG(llvm::dbgs() << "Check " << i << "th param:\n"; Ith->dump(llvm::dbgs()); llvm::dbgs() << '\n';); - MixupTag Mixup = howPossibleToMixUpAtCallSite( - First->getType(), Ith->getType(), Ctx, CVRMixPossible); - LLVM_DEBUG(llvm::dbgs() << "\tMixup? " << static_cast(Mixup) << "\n";); - Mixup = sanitiseMixup(Mixup); - LLVM_DEBUG(llvm::dbgs() << "\tMixup (after sanitise)? " - << static_cast(Mixup) << "\n";); - if (Mixup == MIXUP_None) - break; + if (!ImplicitConversion) { + // If implicit conversion check is not enabled, the mixup logic is easy, + // as we only need to check each subsequent argument against the first one + // of the range - mixes between elements of the range can be inferred + // from a mix with the first one, as only references, qualifiers, and + // typedefs are resolved. + Mixup M = CalcMixupBetween(First, Ith); + if (M.Data.Flag == MIXUP_None) + // If no mixup is possible, we are at the end of the current range. + break; + APR.Mixups.emplace_back(M); + } else { + // Calculate mixes with *each* argument in the range. Mixup is calculated + // bidirectional, so only consider the prefix of the currently built + // range. + bool AnyMixupStored = false; + for (unsigned int j = StartIdx; j < i; ++j) { + const ParmVarDecl *Jth = F->getParamDecl(j); + LLVM_DEBUG(llvm::dbgs() << "Check " << j << "th param:\n"; + Jth->dump(llvm::dbgs()); llvm::dbgs() << '\n';); + Mixup M = CalcMixupBetween(Jth, Ith); + if (M.Data.Flag != MIXUP_None) { + AnyMixupStored = true; + APR.Mixups.emplace_back(M); + } + } - APR.emplace_back(Ith, Mixup); - } + if (!AnyMixupStored) + // If there is no any "new" mixup possibility for the Ith param, it + // signifies the end of range. + break; + } - if (APR.size() > 1 && - llvm::any_of(APR, [](const AdjacentParamsMixup::value_type &E) { - return E.second & MIXUP_Typedef; - })) { - // If any of the mixups for argument pairs (1, 2), (1, 3), ... involve a - // typedef match, the first argument might be a typedef too. Set a bit to - // emit a "type alias resolution" diagnostic for the first argument of the - // range. - APR.front().second = MIXUP_Typedef; + // If the loop did not break earlier, another param was found to be mixable. + ++APR.NumParamsChecked; } return APR; @@ -400,6 +961,21 @@ } } +static std::string typeNameAsString(const QualType QT, + const PrintingPolicy &PP) { + std::string Ret; + + { + llvm::raw_string_ostream OS{Ret}; + putTypeName(QT, OS, PP); + } + + if (Ret.empty()) + Ret = ""; + + return Ret; +} + // For convenience, ignore a few well-known parameter variable and type names // which usually show up with repeated parameters. // FIXME: Make this configurable from the checker options. @@ -487,12 +1063,14 @@ : ClangTidyCheck(Name, Context), MinimumLength( clampMinimumLength(Options.get("MinimumLength", DefaultMinLength))), - CVRMixPossible(Options.get("CVRMixPossible", false)) {} + CVRMixPossible(Options.get("CVRMixPossible", false)), + ImplicitConversion(Options.get("ImplicitConversion", false)) {} void AdjacentArgumentsOfSameTypeCheck::storeOptions( ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "MinimumLength", MinimumLength); Options.store(Opts, "CVRMixPossible", CVRMixPossible); + Options.store(Opts, "ImplicitConversion", ImplicitConversion); } void AdjacentArgumentsOfSameTypeCheck::registerMatchers(MatchFinder *Finder) { @@ -536,10 +1114,10 @@ continue; } - AdjacentParamsMixup APR = - paramEqualTypeRange(Fun, ParamEqRangeStartIdx, CVRMixPossible); - ParamEqRangeStartIdx += APR.size(); - if (APR.size() < MinimumLength) + AdjacentParamsMixup APR = paramEqualTypeRange( + Fun, ParamEqRangeStartIdx, CVRMixPossible, ImplicitConversion); + ParamEqRangeStartIdx += APR.NumParamsChecked; + if (APR.NumParamsChecked < MinimumLength) continue; // A function with an adjacent argument range of sufficient length found. @@ -548,64 +1126,162 @@ FunName = ""; const PrintingPolicy &PP{Fun->getASTContext().getLangOpts()}; - std::string FirstArgType = APR.front().first->getType().getAsString(PP); - - const auto HasMixupTagWarrantingTypePrint = - [](const AdjacentParamsMixup::value_type &E) { - return E.second & (MIXUP_Typedef | MIXUP_RefBind | MIXUP_CVR); - }; - - if (llvm::any_of(APR, HasMixupTagWarrantingTypePrint)) { - // If any of the arguments in the range is annotated with a tag that will - // warrant printing type names, the primary diagnostic shouldn't have - // a type name printed. - diag(APR.front().first->getOuterLocStart(), - "%0 adjacent arguments for '%1' of similar type are easily swapped " - "by mistake") - << static_cast(APR.size()) << FunName; - } else { - diag(APR.front().first->getOuterLocStart(), - "%0 adjacent arguments for '%1' of similar type ('%2') are easily " - "swapped by mistake") - << static_cast(APR.size()) << FunName << FirstArgType; + std::string FirstArgType = APR.getFirstParm()->getType().getAsString(PP); + llvm::SmallPtrSet TypePrintedForParm; + + const auto TypePrintFinder = [](const Mixup &E) { + return E.Data.Flag & (MIXUP_Typedef | MIXUP_RefBind | MIXUP_CVR); + }; + bool HasAnyTypePrint = llvm::any_of(APR.Mixups, TypePrintFinder); + const auto ImplicitConvFinder = [](const Mixup &E) { + return E.Data.Flag & MIXUP_Implicit; + }; + bool HasAnyImplicits = llvm::any_of(APR.Mixups, ImplicitConvFinder); + + StringRef MainDiagnostic; + if (HasAnyImplicits) + MainDiagnostic = "%0 adjacent arguments for '%1' of convertible types " + "may be easily swapped " + "by mistake"; + else if (HasAnyTypePrint) + MainDiagnostic = + "%0 adjacent arguments for '%1' of similar type are easily swapped " + "by mistake"; + else + MainDiagnostic = + "%0 adjacent arguments for '%1' of similar type ('%2') are easily " + "swapped by mistake"; + + { + auto Diag = diag(APR.getFirstParm()->getOuterLocStart(), MainDiagnostic) + << static_cast(APR.NumParamsChecked) << FunName; + if (!HasAnyImplicits && !HasAnyTypePrint) + Diag << FirstArgType; } // FIXME: ClangTidy doesn't seem to support emitting a custom source range // highlight without a fixit. This check can't produce useful automatic // fixits, but highlighting the 'CharSourceRange' would be good to have. - diag(APR.back().first->getLocation(), + diag(APR.getLastParm()->getLocation(), "last argument in the adjacent range here", DiagnosticIDs::Note); - for (const auto &ParmPair : APR) { - if (ParmPair.second < MIXUP_Trivial) - llvm_unreachable("Invalid mixup type in result when emitting diags."); - // MIXUP_Trivial: no extra diagnostics required. - - if (HasMixupTagWarrantingTypePrint(ParmPair)) { - std::string ParmType; - { - llvm::raw_string_ostream OS{ParmType}; - LLVM_DEBUG(llvm::dbgs() - << "\n\nGenerating type name for diagnostic...\n\n";); - putTypeName(ParmPair.first->getType(), OS, PP); - } + for (const Mixup &M : APR.Mixups) { + assert(M.Data.Flag >= MIXUP_Trivial && "Too low bits in mixup type."); + // For MIXUP_Trivial no extra diagnostics required. + + std::string FromParmType, ToParmType; + if (TypePrintFinder(M) || ImplicitConvFinder(M)) { + // Typedefs, reference binds, and implicit conversions might result in + // the type of a variable printed in the diagnostic, so we have to + // prepare it. LLVM_DEBUG(llvm::dbgs() - << "Type name generated: " << ParmType << '\n';); + << "\n\nGenerating type names for diagnostic...\n\n";); + FromParmType = typeNameAsString(M.From->getType(), PP); + ToParmType = typeNameAsString(M.To->getType(), PP); + LLVM_DEBUG(llvm::dbgs() << "Type names generated: " << FromParmType + << " >> and << " << ToParmType << '\n';); + } - if (ParmPair.second & MIXUP_Typedef) { + if (TypePrintFinder(M)) { + if (M.Data.Flag & MIXUP_Typedef) { // FIXME: Don't emit one side of the mix in question did not contain a // typedef. - diag(ParmPair.first->getOuterLocStart(), - "after resolving type aliases, type of argument '%0' is '%1'", - DiagnosticIDs::Note) - << ParmPair.first->getName() << ParmType; + if (!TypePrintedForParm.count(M.From)) { + diag(M.From->getOuterLocStart(), + "after resolving type aliases, type of argument '%0' is '%1'", + DiagnosticIDs::Note) + << M.From->getName() << FromParmType; + TypePrintedForParm.insert(M.From); + } + + if (!TypePrintedForParm.count(M.To)) { + diag(M.To->getOuterLocStart(), + "after resolving type aliases, type of argument '%0' is '%1'", + DiagnosticIDs::Note) + << M.To->getName() << ToParmType; + TypePrintedForParm.insert(M.To); + } } - if (ParmPair.second & (MIXUP_RefBind | MIXUP_CVR)) { - diag(ParmPair.first->getOuterLocStart(), + if (M.Data.Flag & (MIXUP_RefBind | MIXUP_CVR)) { + diag(M.To->getOuterLocStart(), "at a call site, '%0' might bind with same force as '%1'", DiagnosticIDs::Note) - << ParmType << FirstArgType; + << ToParmType << FirstArgType; + } + } + + if (ImplicitConvFinder(M)) { + const ConversionSequence <R = M.Data.ConvLTR, &RTL = M.Data.ConvRTL; + + const auto DiagnoseSequence = + [&M, &PP](const std::string &StartType, + const ConversionSequence &Seq) -> std::string { + assert((&Seq == &M.Data.ConvLTR || &Seq == &M.Data.ConvRTL) && + "sequence should be a sequence from the capture."); + llvm::SmallString<256> SS; + llvm::raw_svector_ostream OS{SS}; + + OS << '\'' << StartType << '\''; + if (Seq.StdPre) + OS << " -> '" << typeNameAsString(QualType{Seq.StdPre, 0}, PP) + << '\''; + if (Seq.UserType) + OS << " -> '" << typeNameAsString(QualType{Seq.UserType, 0}, PP) + << '\''; + if (Seq.StdPost) + OS << " -> '" << typeNameAsString(QualType{Seq.StdPost, 0}, PP) + << '\''; + + return OS.str().str(); + }; + + if (LTR && RTL) { + std::string ConvMsg = + "'%0' and '%1' can suffer implicit conversions between one " + "another"; + if (!LTR.single() && !RTL.single()) + ConvMsg.append(": %2 and %3"); + else if (!LTR.single()) + ConvMsg.append(": %2 and trivially"); + else if (!RTL.single()) + ConvMsg.append(": trivially and %2"); + + auto Diag = + diag(M.To->getOuterLocStart(), ConvMsg, DiagnosticIDs::Note) + << FromParmType << ToParmType; + if (!LTR.single() && !RTL.single()) + Diag << DiagnoseSequence(FromParmType, LTR) + << DiagnoseSequence(ToParmType, RTL); + else if (!LTR.single()) + Diag << DiagnoseSequence(FromParmType, LTR); + else if (!RTL.single()) + Diag << DiagnoseSequence(ToParmType, RTL); + } else { + std::string SeqDetails; + StringRef ConvMsg; + if (LTR) { + if (LTR.single()) + ConvMsg = "'%0' can be implicitly converted to '%1'"; + else { + ConvMsg = "'%0' can be implicitly converted to '%1': %2"; + SeqDetails = DiagnoseSequence(FromParmType, LTR); + } + } else if (RTL) { + if (RTL.single()) + ConvMsg = "'%0' can be implicitly converted from '%1'"; + else { + ConvMsg = "'%0' can be implicitly converted from '%1': %2"; + SeqDetails = DiagnoseSequence(ToParmType, RTL); + } + } + assert(!ConvMsg.empty() && "implicit bit set but we arrived here?"); + + auto Diag = + diag(M.To->getOuterLocStart(), ConvMsg, DiagnosticIDs::Note) + << FromParmType << ToParmType; + if (!SeqDetails.empty()) + Diag << SeqDetails; } } } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -115,9 +115,9 @@ ` check. - Finds function definitions where arguments of the same type follow each other - directly, making call sites prone to calling the function with swapped or badly - ordered arguments. + Finds function definitions where arguments of the essentially similar type + follow each other directly, making call sites prone to calling the function + with swapped or badly ordered arguments. - New :doc:`cppcoreguidelines-init-variables ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.rst --- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.rst @@ -3,9 +3,9 @@ cppcoreguidelines-avoid-adjacent-arguments-of-same-type ======================================================= -Finds function definitions where arguments of the same type follow each other -directly, making call sites prone to calling the function with swapped or badly -ordered arguments. +Finds function definitions where arguments of the essentially similar type +follow each other directly, making call sites prone to calling the function +with swapped or badly ordered arguments. The rule is part of the "Interfaces" profile of the C++ Core Guidelines, see `I.24 `_. @@ -76,6 +76,41 @@ struct T {}; void f(T *tp, const T *ctp) {} +.. option:: ImplicitConversion + + Whether to allow the approximation of some usual + `implicit conversions `_ + when considering which type might be potentially mixed up with another. + If `0` (default value), the check does not consider **any** implicit + conversions. + A non-zero value turns this **on**. + A non-zero value will in almost all cases produce a **significantly broader** + set of results. + + .. warning:: + Turning the modelling of implicit conversion sequences on + relaxes the constraints for "type convertibility" significantly, + however, it also applies a generous performance hit on the check's cost. + The check will have to explore a **polynomially more** possibilities: + O(n\ :sup:`2`\ ) instead of O(n) for each function's ``n`` arguments. + The emitted diagnostics will also be more verbose, which might take more + time to stringify. + + It is advised to normally leave this option *off*. + + For details on what is matched by this option, see + `Implicit conversion modelling`_. + + The following examples will not produce a diagnostic unless + *ImplicitConversion* is set to a non-zero value. + + .. code-block:: c++ + + void f(char c, int i, double d) {} + + enum E { Ea, Eb }; + void f(E e, int i, double d) {} + Limitations ----------- @@ -125,3 +160,36 @@ const vector & Vector, typename vector::const_reference_type RightEnd, const typename vector::element_type & LeftBegin) { /* ... */ } + + +Implicit conversion modelling +----------------------------- + +Given a function ``void f(T1 t, T2 u)``, unless ``T1`` and ``T2`` are the same, +the check won't warn in *"default"* mode. If ``ImplicitConversion`` is turned on +(see Options_), and either ``T1`` +`can be converted to `_ +``T2``, or vica versa, diagnostics will be made. +The "adjacent argument mixup" is considered even if the conversion is asymmetric, +which is natural in most cases. + +In the case of the following function, the numeric arguments can be +converted between one another, and the ``SomeEnum`` is convertible to any +numeric type. +Thus, an "adjacent argument range" of **5** is diagnosed here. + +.. code-blocK:: c++ + + enum SomeEnum { /* ... */ }; + void SomeFunction(int, float, double, unsigned long, SomeEnum) {} + +Currently, the following implicit conversions are modelled: + + - *standard conversion* sequences: + - numeric promotion and conversion between integer (including ``enum``) and + floating types, considered essentially the same under the umbrella term + "conversion" + - For **C** projects, the numeric conversion rules are relaxed to conform + to C rules. + - *user defined conversions*: non-``explicit`` converting constructors and + conversion operators. diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-cvr-on.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-cvr-on.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-cvr-on.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-cvr-on.cpp @@ -2,7 +2,8 @@ // RUN: cppcoreguidelines-avoid-adjacent-arguments-of-same-type %t \ // RUN: -config='{CheckOptions: [ \ // RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.MinimumLength, value: 3}, \ -// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 1} \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 1}, \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.ImplicitConversion, value: 0} \ // RUN: ]}' -- struct T {}; @@ -51,6 +52,4 @@ Numeral *const value) {} // CHECK-MESSAGES: :[[@LINE-3]]:17: warning: 3 adjacent arguments for 'set_lerped' of similar type are // CHECK-MESSAGES: :[[@LINE-2]]:32: note: last argument in the adjacent range -// CHECK-MESSAGES: :[[@LINE-5]]:17: note: after resolving type aliases, type of argument 'min' is 'const int * const' -// CHECK-MESSAGES: :[[@LINE-4]]:17: note: after resolving type aliases, type of argument 'value' is 'int * const' -// CHECK-MESSAGES: :[[@LINE-5]]:17: note: at a call site, 'int * const' might bind with same force as 'const Numeral *const' +// CHECK-MESSAGES: :[[@LINE-3]]:17: note: at a call site, 'int * const' might bind with same force as 'const Numeral *const' diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-default.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-default.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-default.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-default.cpp @@ -2,7 +2,8 @@ // RUN: cppcoreguidelines-avoid-adjacent-arguments-of-same-type %t \ // RUN: -config='{CheckOptions: [ \ // RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.MinimumLength, value: 3}, \ -// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 0} \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 0}, \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.ImplicitConversion, value: 0} \ // RUN: ]}' -- void library(void *vp, void *vp2, void *vp3, int n, int m); @@ -105,6 +106,12 @@ void value_ptr_quals(T *tp, const T *ctp, volatile T *vtp, const volatile T *cvtp, T *const tpc, T *volatile tpv) {} +void typedef_and_realtype_ptr_const(int *p1, I1 *p2, const I1 *p3) {} + +void typedef_ptr_const(I1 *p1, I1 *p2, const I1 *p3) {} + +// END of NO-WARN. + // NO-WARN: const T* and T* should not mix, even if coming through a typedef. // (min and max could be mixed up, but length requirement in this test is 3.) typedef int Numeral; @@ -112,8 +119,6 @@ const Numeral *const max, Numeral *const value) {} -// END of NO-WARN. - void protochain(int, int, int); void protochain(int i, int, int); void protochain(int, int j, int); @@ -191,3 +196,20 @@ // CHECK-MESSAGES: :[[@LINE-5]]:25: note: after resolving type aliases, type of argument 'Background' is 'OldSchoolTermColour' // CHECK-MESSAGES: :[[@LINE-5]]:25: note: after resolving type aliases, type of argument 'Foreground' is 'OldSchoolTermColour' // CHECK-MESSAGES: :[[@LINE-5]]:25: note: after resolving type aliases, type of argument 'Border' is 'OldSchoolTermColour' + +// NO-WARN: Implicit conversions should not warn if the check option is turned off. + +void integral1(signed char sc, int si) {} + +struct FromInt { + FromInt(int); +}; +void userConv1(int i, FromInt fi) {} + +struct Base {}; +struct Derived1 : Base {}; +struct Derived2 : Base {}; +void upcasting(Base *bp, Derived1 *d1p, Derived2 *d2p) {} +void upcasting_ref(const Base &br, const Derived1 &d1r, const Derived2 &d2r) {} + +// END of NO-WARN. diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-implicits.c b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-implicits.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-implicits.c @@ -0,0 +1,90 @@ +// RUN: %check_clang_tidy %s \ +// RUN: cppcoreguidelines-avoid-adjacent-arguments-of-same-type %t \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.MinimumLength, value: 2}, \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 0}, \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.ImplicitConversion, value: 1} \ +// RUN: ]}' -- + +void compare(int a, int b, int c) {} +// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 3 adjacent arguments for 'compare' of similar type ('int') are easily swapped by mistake [cppcoreguidelines-avoid-adjacent-arguments-of-same-type] +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: last argument in the adjacent range here + +void b(_Bool b1, _Bool b2, int i) {} +// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 3 adjacent arguments for 'b' of convertible types may be easily swapped by mistake [cppcoreguidelines-avoid-adjacent-arguments-of-same-type] +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: last argument in the adjacent range here +// CHECK-MESSAGES: :[[@LINE-3]]:28: note: '_Bool' and 'int' can suffer implicit conversions between one another{{$}} + +void integral1(signed char sc, int si) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'integral1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:32: note: 'signed char' and 'int' can suffer implicit conversions between one another{{$}} + +void integral2(long l, int i) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'integral2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:28: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:24: note: 'long' and 'int' can suffer implicit conversions between one another{{$}} + +void integral3(int i, long l) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'integral3' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:28: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:23: note: 'int' and 'long' can suffer implicit conversions between one another{{$}} + +void integral4(char c, int i, long l) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 3 adjacent arguments for 'integral4' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:24: note: 'char' and 'int' can suffer implicit conversions between one another{{$}} +// CHECK-MESSAGES: :[[@LINE-4]]:31: note: 'char' and 'long' can suffer implicit conversions between one another{{$}} + +void floating1(float f, double d) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'floating1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:25: note: 'float' and 'double' can suffer implicit conversions between one another{{$}} + +typedef double D; +void floating2(float f, D d) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'floating2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:27: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:16: note: after resolving type aliases, type of argument 'f' is 'float' +// CHECK-MESSAGES: :[[@LINE-4]]:25: note: after resolving type aliases, type of argument 'd' is 'double' +// CHECK-MESSAGES: :[[@LINE-5]]:25: note: 'float' and 'double' can suffer implicit conversions between one another{{$}} + +void floatToInt(float f, int i) {} +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent arguments for 'floatToInt' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:30: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:26: note: 'float' and 'int' can suffer implicit conversions between one another{{$}} + +enum En { A, + B }; + +void unscopedEnumToIntegral(enum En e, int i) {} +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent arguments for 'unscopedEnumToIntegral' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:44: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:40: note: 'enum En' and 'int' can suffer implicit conversions between one another{{$}} + +void unscopedEnumToIntegral2(enum En e, unsigned long long ull) {} +// CHECK-MESSAGES: :[[@LINE-1]]:30: warning: 2 adjacent arguments for 'unscopedEnumToIntegral2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:60: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:41: note: 'enum En' and 'unsigned long long' can suffer implicit conversions between one another{{$}} + +void unscopedEnum3(char c, enum En e, char c2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 3 adjacent arguments for 'unscopedEnum3' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:44: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:28: note: 'char' and 'enum En' can suffer implicit conversions between one another{{$}} +// CHECK-MESSAGES: :[[@LINE-4]]:39: note: 'enum En' and 'char' can suffer implicit conversions between one another{{$}} + +void unscopedEnumToFloating(enum En e, long double ld) {} +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent arguments for 'unscopedEnumToFloating' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:52: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:40: note: 'enum En' and 'long double' can suffer implicit conversions between one another{{$}} + +void unscopedEnumToIntOrFloat(enum En e, int i, float f) {} +// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 3 adjacent arguments for 'unscopedEnumToIntOrFloat' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:55: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:42: note: 'enum En' and 'int' can suffer implicit conversions between one another{{$}} +// CHECK-MESSAGES: :[[@LINE-4]]:49: note: 'enum En' and 'float' can suffer implicit conversions between one another{{$}} +// CHECK-MESSAGES: :[[@LINE-5]]:49: note: 'int' and 'float' can suffer implicit conversions between one another{{$}} + +void ptr(int *i, long *l) {} +// NO-WARN: Though 'int' and 'long' can be converted between one-another +// implicitly, mixing such pointers is diagnosed by compiler warnings. diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-implicits.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-implicits.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-implicits.cpp @@ -0,0 +1,330 @@ +// RUN: %check_clang_tidy %s \ +// RUN: cppcoreguidelines-avoid-adjacent-arguments-of-same-type %t \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.MinimumLength, value: 2}, \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 0}, \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.ImplicitConversion, value: 1} \ +// RUN: ]}' -- + +void compare(int a, int b, int c) {} +// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 3 adjacent arguments for 'compare' of similar type ('int') are easily swapped by mistake [cppcoreguidelines-avoid-adjacent-arguments-of-same-type] +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: last argument in the adjacent range here + +void b(bool b1, bool b2, int i) {} +// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 3 adjacent arguments for 'b' of convertible types may be easily swapped by mistake [cppcoreguidelines-avoid-adjacent-arguments-of-same-type] +// CHECK-MESSAGES: :[[@LINE-2]]:30: note: last argument in the adjacent range here +// CHECK-MESSAGES: :[[@LINE-3]]:26: note: 'bool' and 'int' can suffer implicit conversions between one another{{$}} + +void integral1(signed char sc, int si) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'integral1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:32: note: 'signed char' and 'int' can suffer implicit conversions between one another{{$}} + +void integral2(long l, int i) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'integral2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:28: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:24: note: 'long' and 'int' can suffer implicit conversions between one another{{$}} + +void integral3(int i, long l) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'integral3' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:28: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:23: note: 'int' and 'long' can suffer implicit conversions between one another{{$}} + +void integral4(char c, int i, long l) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 3 adjacent arguments for 'integral4' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:24: note: 'char' and 'int' can suffer implicit conversions between one another{{$}} +// CHECK-MESSAGES: :[[@LINE-4]]:31: note: 'char' and 'long' can suffer implicit conversions between one another{{$}} + +void floating1(float f, double d) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'floating1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:32: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:25: note: 'float' and 'double' can suffer implicit conversions between one another{{$}} + +typedef double D; +void floating2(float f, D d) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'floating2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:27: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:16: note: after resolving type aliases, type of argument 'f' is 'float' +// CHECK-MESSAGES: :[[@LINE-4]]:25: note: after resolving type aliases, type of argument 'd' is 'double' +// CHECK-MESSAGES: :[[@LINE-5]]:25: note: 'float' and 'double' can suffer implicit conversions between one another{{$}} + +void floatToInt(float f, int i) {} +// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent arguments for 'floatToInt' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:30: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:26: note: 'float' and 'int' can suffer implicit conversions between one another{{$}} + +enum En { A, + B }; + +void unscopedEnumToIntegral(En e, int i) {} +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent arguments for 'unscopedEnumToIntegral' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:39: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:35: note: 'En' can be implicitly converted to 'int' + +void unscopedEnumToIntegral2(En e, unsigned long long ull) {} +// CHECK-MESSAGES: :[[@LINE-1]]:30: warning: 2 adjacent arguments for 'unscopedEnumToIntegral2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:55: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:36: note: 'En' can be implicitly converted to 'unsigned long long' + +void unscopedEnum3(char c, En e, char c2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 3 adjacent arguments for 'unscopedEnum3' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:39: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:28: note: 'char' can be implicitly converted from 'En' +// CHECK-MESSAGES: :[[@LINE-4]]:34: note: 'En' can be implicitly converted to 'char' + +void unscopedEnumToFloating(En e, long double ld) {} +// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: 2 adjacent arguments for 'unscopedEnumToFloating' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:47: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:35: note: 'En' can be implicitly converted to 'long double' + +void unscopedEnumToIntOrFloat(En e, int i, float f) {} +// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 3 adjacent arguments for 'unscopedEnumToIntOrFloat' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:50: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:37: note: 'En' can be implicitly converted to 'int' +// CHECK-MESSAGES: :[[@LINE-4]]:44: note: 'En' can be implicitly converted to 'float' +// CHECK-MESSAGES: :[[@LINE-5]]:44: note: 'int' and 'float' can suffer implicit conversions between one another{{$}} + +enum class SEn { C, + D }; +void scopedEnumToIntegral(SEn e, int i) {} +// NO-WARN: Scoped enumerations mustn't be promoted. + +struct FromInt { + FromInt(int); +}; + +void userConv1(int i, FromInt fi) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'userConv1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:31: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:23: note: 'int' can be implicitly converted to 'FromInt' + +void userConv1c(int i, const FromInt cfi) {} +// NO-WARN: CVRMixPossible is set to 0, so an 'int' and a 'const "int"' does not mix. + +void userConv1cr(int i, const FromInt &cfir) {} +// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 2 adjacent arguments for 'userConv1cr' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:40: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:25: note: at a call site, 'const FromInt &' might bind with same force as 'int' +// CHECK-MESSAGES: :[[@LINE-4]]:25: note: 'int' can be implicitly converted to 'const FromInt &' + +void userConv2(unsigned long long i, FromInt fi) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'userConv2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:46: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:38: note: 'unsigned long long' can be implicitly converted to 'FromInt': 'unsigned long long' -> 'int' -> 'FromInt' + +struct ToInt { + operator int() const; +}; + +void userConv3(int i, ToInt ti) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'userConv3' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:29: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:23: note: 'int' can be implicitly converted from 'ToInt' + +void userConv4(double d, FromInt fi) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'userConv4' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:34: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:26: note: 'double' can be implicitly converted to 'FromInt': 'double' -> 'int' -> 'FromInt' + +void userConv4cr(double d, const FromInt &cfir) {} +// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 2 adjacent arguments for 'userConv4cr' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:43: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:28: note: at a call site, 'const FromInt &' might bind with same force as 'double' +// CHECK-MESSAGES: :[[@LINE-4]]:28: note: 'double' can be implicitly converted to 'const FromInt &': 'double' -> 'int' -> 'FromInt' + +struct FromDouble { + FromDouble(double); +}; + +void userConv5(En e, FromDouble fd) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'userConv5' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:33: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:22: note: 'En' can be implicitly converted to 'FromDouble': 'En' -> 'double' -> 'FromDouble' + +struct Ambiguous { + Ambiguous(int); + Ambiguous(long); +}; + +void userConv6(En e, Ambiguous a) {} +// NO-WARN: En -> int -> Ambiguous vs. En -> long -> Ambiguous. + +struct ToExplicitInt { + explicit operator int() const; +}; +struct FromExplicitInt { + explicit FromExplicitInt(int); +}; + +void nonConverting1(int i, FromExplicitInt fei) {} +// NO-WARN: There is no chance of mix-up as 'fei' has only explicit constructor. + +void nonConverting2(int i, ToExplicitInt tei) {} +// NO-WARN: 'tei' converts to 'int' only in explicit context. + +struct Both { + Both(int); + operator int() const; +}; + +typedef int I; +typedef double D; + +void both1(int i, Both b) {} +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent arguments for 'both1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:24: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:19: note: 'int' and 'Both' can suffer implicit conversions between one another{{$}} + +void both2(double d, Both b) {} +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent arguments for 'both2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:27: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:22: note: 'double' and 'Both' can suffer implicit conversions between one another: 'double' -> 'int' -> 'Both' and 'Both' -> 'int' -> 'double' + +void both2typedef(D d, Both b) {} +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent arguments for 'both2typedef' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:29: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:19: note: after resolving type aliases, type of argument 'd' is 'double' +// CHECK-MESSAGES: :[[@LINE-4]]:24: note: after resolving type aliases, type of argument 'b' is 'Both' +// CHECK-MESSAGES: :[[@LINE-5]]:24: note: 'double' and 'Both' can suffer implicit conversions between one another: 'double' -> 'int' -> 'Both' and 'Both' -> 'int' -> 'double' + +struct BoxedInt { + BoxedInt(); + BoxedInt(const BoxedInt &); + BoxedInt(BoxedInt &&); + BoxedInt(const Both &B); +}; + +void both3(int i, BoxedInt biv) {} +// NO-WARN: Two converting constructor calls (int->both->BoxedInt) is not +// possible implicitly. + +struct IntAndDouble { + IntAndDouble(const int); + IntAndDouble(const double); + + operator int() const; + operator double() const; +}; + +void twoTypes1(int i, IntAndDouble iad) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'twoTypes1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:36: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:23: note: 'int' and 'IntAndDouble' can suffer implicit conversions between one another{{$}} + +void twoTypes2(double d, IntAndDouble iad) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 2 adjacent arguments for 'twoTypes2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:39: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:26: note: 'double' and 'IntAndDouble' can suffer implicit conversions between one another{{$}} + +void twoTypes3(unsigned long ul, IntAndDouble iad) {} +// NO-WARN: Ambiguous constructor call and conversion call, should we take +// 'int' or 'double' route from 'unsigned long'? + +struct IADTypedef { + IADTypedef(I); + IADTypedef(D); + operator I() const; + operator D() const; +}; + +void twoTypedefs1(int i, IADTypedef iadt) {} +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent arguments for 'twoTypedefs1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:37: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:26: note: 'int' and 'IADTypedef' can suffer implicit conversions between one another{{$}} + +void twoTypedefs2(double d, IADTypedef iadt) {} +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent arguments for 'twoTypedefs2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:40: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:29: note: 'double' and 'IADTypedef' can suffer implicit conversions between one another{{$}} + +void twoTypedefs3(unsigned long ul, IADTypedef iadt) {} +// NO-WARN: Ambiguous constructor call and conversion call, should we take +// 'int' or 'double' route from 'unsigned long'? + +struct Fwd1; +struct Fwd2; +void forwards(Fwd1 *f1, Fwd2 *f2) {} +// NO-WARN and NO-CRASH: Don't try to compare not complete types. + +struct Rec {}; +struct FromRec { + FromRec(const Rec &); +}; +struct ToRec { + operator Rec() const; +}; +struct RecBoth { + RecBoth(Rec); + operator Rec() const; +}; + +void record2record_1(Rec r, FromRec fr) {} +// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 2 adjacent arguments for 'record2record_1' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:37: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:29: note: 'Rec' can be implicitly converted to 'FromRec' + +void record2record_2(Rec r, ToRec tr) {} +// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 2 adjacent arguments for 'record2record_2' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:35: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:29: note: 'Rec' can be implicitly converted from 'ToRec' + +void record2record_3(Rec r, RecBoth rb) {} +// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 2 adjacent arguments for 'record2record_3' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:37: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:29: note: 'Rec' and 'RecBoth' can suffer implicit conversions between one another{{$}} + +void record2record_4(Rec r, FromRec fr, ToRec tr, RecBoth rb) {} +// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 4 adjacent arguments for 'record2record_4' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:59: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:29: note: 'Rec' can be implicitly converted to 'FromRec' +// CHECK-MESSAGES: :[[@LINE-4]]:41: note: 'Rec' can be implicitly converted from 'ToRec' +// CHECK-MESSAGES: :[[@LINE-5]]:51: note: 'Rec' and 'RecBoth' can suffer implicit conversions between one another{{$}} + +struct X; +struct Y; +struct X { + X(); + X(Y); + operator Y(); +}; +struct Y { + Y(); + Y(X); + operator X(); +}; +void ambiguous_records(X x, Y y) {} +// NO-WARN: Ambiguous conversion if the arguments are swapped, which results in +// compiler error: f3(Y{}, X{}) + +struct Base {}; +struct Derived1 : Base {}; +struct Derived2 : Base {}; +void upcasting(Base *bp, Derived1 *d1p, Derived2 *d2p) {} +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 3 adjacent arguments for 'upcasting' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:51: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:26: note: 'Base *' can be implicitly converted from 'Derived1 *' +// CHECK-MESSAGES: :[[@LINE-4]]:41: note: 'Base *' can be implicitly converted from 'Derived2 *' + +void upcasting_ref(const Base &br, const Derived1 &d1r, const Derived2 &d2r) {} +// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 3 adjacent arguments for 'upcasting_ref' of convertible +// CHECK-MESSAGES: :[[@LINE-2]]:73: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:36: note: at a call site, 'const Derived1 &' might bind with same force as 'const Base &' +// CHECK-MESSAGES: :[[@LINE-4]]:36: note: 'const Base &' can be implicitly converted from 'const Derived1 &' +// CHECK-MESSAGES: :[[@LINE-5]]:57: note: at a call site, 'const Derived2 &' might bind with same force as 'const Base &' +// CHECK-MESSAGES: :[[@LINE-6]]:57: note: 'const Base &' can be implicitly converted from 'const Derived2 &' + +// Reduced case from live crash on OpenCV +// http://github.com/opencv/opencv/blob/1996ae4a42d7c7cd338fbdd4abbd83b41b448328/modules/core/include/opencv2/core/types.hpp#L173 +template +struct TemplateConversion { + TemplateConversion(); + TemplateConversion(const T &); + template + operator TemplateConversion() const; +}; +typedef TemplateConversion IntConvert; +typedef TemplateConversion FloatConvert; +void templated_conversion_to_other_specialisation(FloatConvert fc, IntConvert ic) {} +// NO-WARN: Template conversion operators are not resolved, we have approximated +// much of Sema already... diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-verbose.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-verbose.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-verbose.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type-verbose.cpp @@ -2,7 +2,8 @@ // RUN: cppcoreguidelines-avoid-adjacent-arguments-of-same-type %t \ // RUN: -config='{CheckOptions: [ \ // RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.MinimumLength, value: 2}, \ -// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 1} \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 1}, \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.ImplicitConversion, value: 0} \ // RUN: ]}' -- void library(void *vp, void *vp2, void *vp3, int n, int m); @@ -124,6 +125,18 @@ // CHECK-MESSAGES: :[[@LINE-3]]:32: note: after resolving type aliases, type of argument 'p1' is 'int *' // CHECK-MESSAGES: :[[@LINE-4]]:40: note: after resolving type aliases, type of argument 'p2' is 'int *' +void typedef_and_realtype_ptr_const(int *p1, const I1 *p2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 2 adjacent arguments for 'typedef_and_realtype_ptr_const' of similar type are +// CHECK-MESSAGES: :[[@LINE-2]]:56: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:37: note: after resolving type aliases, type of argument 'p1' is 'int *' +// CHECK-MESSAGES: :[[@LINE-4]]:46: note: after resolving type aliases, type of argument 'p2' is 'const int *' +// CHECK-MESSAGES: :[[@LINE-5]]:46: note: at a call site, 'const int *' might bind with same force as 'int *' + +void typedef_ptr_const(I1 *p1, const I1 *p2) {} +// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent arguments for 'typedef_ptr_const' of similar type are +// CHECK-MESSAGES: :[[@LINE-2]]:42: note: last argument in the adjacent range +// CHECK-MESSAGES: :[[@LINE-3]]:32: note: at a call site, 'const int *' might bind with same force as 'I1 *' + typedef I2 myInt; typedef I2 myOtherInt; void typedef_chain(myInt mi, myOtherInt moi) {} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.c b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.c --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.c +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.c @@ -2,7 +2,8 @@ // RUN: cppcoreguidelines-avoid-adjacent-arguments-of-same-type %t \ // RUN: -config='{CheckOptions: [ \ // RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.MinimumLength, value: 2}, \ -// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 1} \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.CVRMixPossible, value: 1}, \ +// RUN: {key: cppcoreguidelines-avoid-adjacent-arguments-of-same-type.ImplicitConversion, value: 0} \ // RUN: ]}' -- struct T {}; @@ -29,3 +30,6 @@ int equals2(S l, S r) { return l.x == r.x; } // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent arguments for 'equals2' of similar type ('S') are // CHECK-MESSAGES: :[[@LINE-2]]:20: note: last argument in the adjacent range + +void ptrs(int *i, long *l) {} +// NO-WARN: Mixing fundamentals' pointers is diagnosed by compiler warnings.