diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp @@ -469,18 +469,18 @@ return *this; } - /// Add the specified qualifiers to the common type in the Mix. - MixData qualify(Qualifiers Quals, const ASTContext &Ctx) const { + template MixData withCommonTypeTransformed(F &&Func) const { if (CommonType.isNull()) return *this; - QualType NewCommonType = Ctx.getQualifiedType(CommonType, Quals); + QualType NewCommonType = Func(CommonType); if (CreatedFromOneWayConversion) { MixData M{Flags, Conversion}; M.CommonType = NewCommonType; return M; } + return {Flags, NewCommonType, Conversion, ConversionRTL}; } }; @@ -544,13 +544,13 @@ /// Helper enum for the recursive calls in the modelling that toggle what kinds /// of implicit conversions are to be modelled. enum class ImplicitConversionModellingMode : unsigned char { - /// No implicit conversions are modelled. + ///< No implicit conversions are modelled. None, - /// The full implicit conversion sequence is modelled. + ///< The full implicit conversion sequence is modelled. All, - /// Only model a unidirectional implicit conversion and within it only one + ///< Only model a unidirectional implicit conversion and within it only one /// standard conversion sequence. OneWaySingleStandardOnly }; @@ -570,17 +570,30 @@ return isa(T); } +namespace { + +struct NonCVRQualifiersResult { + /// True if the types are qualified in a way that even after equating or + /// removing local CVR qualification, even if the unqualified types + /// themselves would mix, the qualified ones don't, because there are some + /// other local qualifiers that are not equal. + bool HasMixabilityBreakingQualifiers; + + /// The set of equal qualifiers between the two types. + Qualifiers CommonQualifiers; +}; + +} // namespace + /// Returns if the two types are qualified in a way that ever after equating or /// removing local CVR qualification, even if the unqualified types would mix, /// the qualified ones don't, because there are some other local qualifiers /// that aren't equal. -static bool hasNonCVRMixabilityBreakingQualifiers(const ASTContext &Ctx, - QualType LType, - QualType RType) { - LLVM_DEBUG( - llvm::dbgs() << ">>> hasNonCVRMixabilityBreakingQualifiers for LType:\n"; - LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n"; - RType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';); +static NonCVRQualifiersResult +getNonCVRQualifiers(const ASTContext &Ctx, QualType LType, QualType RType) { + LLVM_DEBUG(llvm::dbgs() << ">>> getNonCVRQualifiers for LType:\n"; + LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n"; + RType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';); Qualifiers LQual = LType.getLocalQualifiers(), RQual = RType.getLocalQualifiers(); @@ -588,20 +601,24 @@ LQual.removeCVRQualifiers(); RQual.removeCVRQualifiers(); - Qualifiers CommonQuals = Qualifiers::removeCommonQualifiers(LQual, RQual); - (void)CommonQuals; + NonCVRQualifiersResult Ret; + Ret.CommonQualifiers = Qualifiers::removeCommonQualifiers(LQual, RQual); LLVM_DEBUG(llvm::dbgs() << "--- hasNonCVRMixabilityBreakingQualifiers. " "Removed common qualifiers: "; - CommonQuals.print(llvm::dbgs(), Ctx.getPrintingPolicy()); + Ret.CommonQualifiers.print(llvm::dbgs(), Ctx.getPrintingPolicy()); llvm::dbgs() << "\n\tremaining on LType: "; LQual.print(llvm::dbgs(), Ctx.getPrintingPolicy()); llvm::dbgs() << "\n\tremaining on RType: "; RQual.print(llvm::dbgs(), Ctx.getPrintingPolicy()); llvm::dbgs() << '\n';); - // If there are any remaining qualifiers, consider the two types distinct. - return LQual.hasQualifiers() || RQual.hasQualifiers(); + // If there are no other non-cvr non-common qualifiers left, we can deduce + // that mixability isn't broken. + Ret.HasMixabilityBreakingQualifiers = + LQual.hasQualifiers() || RQual.hasQualifiers(); + + return Ret; } /// Approximate the way how LType and RType might refer to "essentially the @@ -641,18 +658,28 @@ Check, LType, RType.getSingleStepDesugaredType(Ctx), Ctx, ImplicitMode); } + const auto *LLRef = LType->getAs(); + const auto *RLRef = RType->getAs(); + if (LLRef && RLRef) { + LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS and RHS are &.\n"); + + return calculateMixability(Check, LLRef->getPointeeType(), + RLRef->getPointeeType(), Ctx, ImplicitMode) + .withCommonTypeTransformed( + [&Ctx](QualType QT) { return Ctx.getLValueReferenceType(QT); }); + } // At a particular call site, what could be passed to a 'T' or 'const T' might // also be passed to a 'const T &' without the call site putting a direct // side effect on the passed expressions. - if (const auto *LRef = LType->getAs()) { + if (LLRef) { LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is &.\n"); - return isLRefEquallyBindingToType(Check, LRef, RType, Ctx, false, + return isLRefEquallyBindingToType(Check, LLRef, RType, Ctx, false, ImplicitMode) | MixFlags::ReferenceBind; } - if (const auto *RRef = RType->getAs()) { + if (RLRef) { LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is &.\n"); - return isLRefEquallyBindingToType(Check, RRef, LType, Ctx, true, + return isLRefEquallyBindingToType(Check, RLRef, LType, Ctx, true, ImplicitMode) | MixFlags::ReferenceBind; } @@ -676,8 +703,6 @@ // // Whether to do this check for the inner unqualified types. bool CompareUnqualifiedTypes = false; - // Should the result have a common inner type qualified for diagnosis? - bool RequalifyUnqualifiedMixabilityResult = false; if (LType.getLocalCVRQualifiers() != RType.getLocalCVRQualifiers()) { LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) { llvm::dbgs() << "--- calculateMixability. LHS has CVR-Qualifiers: "; @@ -701,6 +726,8 @@ CompareUnqualifiedTypes = true; } + // Whether the two types had the same CVR qualifiers. + bool OriginallySameQualifiers = false; if (LType.getLocalCVRQualifiers() == RType.getLocalCVRQualifiers() && LType.getLocalCVRQualifiers() != 0) { LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) { @@ -712,11 +739,13 @@ }); CompareUnqualifiedTypes = true; - RequalifyUnqualifiedMixabilityResult = true; + OriginallySameQualifiers = true; } if (CompareUnqualifiedTypes) { - if (hasNonCVRMixabilityBreakingQualifiers(Ctx, LType, RType)) { + NonCVRQualifiersResult AdditionalQuals = + getNonCVRQualifiers(Ctx, LType, RType); + if (AdditionalQuals.HasMixabilityBreakingQualifiers) { LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Additional " "non-equal incompatible qualifiers.\n"); return {MixFlags::None}; @@ -724,14 +753,23 @@ MixData UnqualifiedMixability = calculateMixability(Check, LType.getLocalUnqualifiedType(), - RType.getLocalUnqualifiedType(), Ctx, ImplicitMode); - - if (!RequalifyUnqualifiedMixabilityResult) + RType.getLocalUnqualifiedType(), Ctx, ImplicitMode) + .withCommonTypeTransformed([&AdditionalQuals, &Ctx](QualType QT) { + // Once the mixability was deduced, apply the qualifiers common + // to the two type back onto the diagnostic printout. + return Ctx.getQualifiedType(QT, AdditionalQuals.CommonQualifiers); + }); + + if (!OriginallySameQualifiers) + // User-enabled qualifier change modelled for the mix. return UnqualifiedMixability | MixFlags::Qualifiers; - // Apply the same qualifier back into the found common type if we found - // a common type between the unqualified versions. - return UnqualifiedMixability.qualify(LType.getLocalQualifiers(), Ctx); + // Apply the same qualifier back into the found common type if they were + // the same. + return UnqualifiedMixability.withCommonTypeTransformed( + [&Ctx, LType](QualType QT) { + return Ctx.getQualifiedType(QT, LType.getLocalQualifiers()); + }); } // Certain constructs match on the last catch-all getCanonicalType() equality, @@ -745,9 +783,12 @@ // some other match. However, this must not consider implicit conversions. LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS and RHS are Ptrs.\n"); - MixData MixOfPointee = calculateMixability( - Check, LType->getPointeeType(), RType->getPointeeType(), Ctx, - ImplicitConversionModellingMode::None); + MixData MixOfPointee = + calculateMixability(Check, LType->getPointeeType(), + RType->getPointeeType(), Ctx, + ImplicitConversionModellingMode::None) + .withCommonTypeTransformed( + [&Ctx](QualType QT) { return Ctx.getPointerType(QT); }); if (hasFlag(MixOfPointee.Flags, MixFlags::WorkaroundDisableCanonicalEquivalence)) RecursiveReturnDiscardingCanonicalType = true; @@ -2193,14 +2234,14 @@ UniqueTypeAlias(LType, RType, CommonType)) { StringRef DiagText; bool ExplicitlyPrintCommonType = false; - if (LTypeStr == CommonTypeStr || RTypeStr == CommonTypeStr) + if (LTypeStr == CommonTypeStr || RTypeStr == CommonTypeStr) { if (hasFlag(M.flags(), MixFlags::Qualifiers)) DiagText = "after resolving type aliases, '%0' and '%1' share a " "common type"; else DiagText = "after resolving type aliases, '%0' and '%1' are the same"; - else if (!CommonType.isNull()) { + } else if (!CommonType.isNull()) { DiagText = "after resolving type aliases, the common type of '%0' " "and '%1' is '%2'"; ExplicitlyPrintCommonType = true; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp @@ -242,8 +242,7 @@ // CHECK-MESSAGES: :[[@LINE-4]]:37: note: 'int' and 'ICRTy' parameters accept and bind the same kind of values // CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, 'int' and 'MyIntCRTy' are the same // CHECK-MESSAGES: :[[@LINE-6]]:52: note: 'int' and 'MyIntCRTy' parameters accept and bind the same kind of values -// CHECK-MESSAGES: :[[@LINE-7]]:37: note: after resolving type aliases, the common type of 'ICRTy' and 'MyIntCRTy' is 'int' -// CHECK-MESSAGES: :[[@LINE-8]]:52: note: 'ICRTy' and 'MyIntCRTy' parameters accept and bind the same kind of values +// CHECK-MESSAGES: :[[@LINE-7]]:37: note: after resolving type aliases, the common type of 'ICRTy' and 'MyIntCRTy' is 'const int &' short const typedef int unsigned Eldritch; typedef const unsigned short Holy; @@ -358,10 +357,20 @@ // CHECK-MESSAGES: :[[@LINE-2]]:30: warning: 2 adjacent parameters of 'attributedParam1Typedef' of similar type are // CHECK-MESSAGES: :[[@LINE-3]]:77: note: the first parameter in the range is 'One' // CHECK-MESSAGES: :[[@LINE-3]]:80: note: the last parameter in the range is 'Two' -// CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, the common type of 'const __attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' is 'const __attribute__((address_space(256))) int' -// FIXME: The last diagnostic line is a bit bad: the common type should be a -// pointer type -- it is not clear right now, how it would be possible to -// properly wire a logic in that fixes it. +// CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, 'const __attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' are the same + +void attributedParam1TypedefRef( + const __attribute__((address_space(256))) int &OneR, + __attribute__((address_space(256))) MyInt1 &TwoR) {} +// NO-WARN: One is CVR-qualified, the other is not. + +void attributedParam1TypedefCRef( + const __attribute__((address_space(256))) int &OneR, + const __attribute__((address_space(256))) MyInt1 &TwoR) {} +// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 2 adjacent parameters of 'attributedParam1TypedefCRef' of similar type are +// CHECK-MESSAGES: :[[@LINE-3]]:52: note: the first parameter in the range is 'OneR' +// CHECK-MESSAGES: :[[@LINE-3]]:55: note: the last parameter in the range is 'TwoR' +// CHECK-MESSAGES: :[[@LINE-5]]:5: note: after resolving type aliases, 'const __attribute__((address_space(256))) int &' and 'const __attribute__((address_space(256))) MyInt1 &' are the same void attributedParam2(__attribute__((address_space(256))) int *One, const __attribute__((address_space(256))) MyInt1 *Two) {} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp @@ -114,13 +114,19 @@ // CHECK-MESSAGES: :[[@LINE-3]]:29: note: the last parameter in the range is 'Source' // CHECK-MESSAGES: :[[@LINE-4]]:26: note: 'const T *' and 'T *' parameters accept and bind the same kind of values +void attributedParam1TypedefRef( + const __attribute__((address_space(256))) int &OneR, + __attribute__((address_space(256))) MyInt1 &TwoR) {} +// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: 2 adjacent parameters of 'attributedParam1TypedefRef' of similar type are +// CHECK-MESSAGES: :[[@LINE-3]]:52: note: the first parameter in the range is 'OneR' +// CHECK-MESSAGES: :[[@LINE-3]]:49: note: the last parameter in the range is 'TwoR' +// CHECK-MESSAGES: :[[@LINE-5]]:5: note: after resolving type aliases, the common type of 'const __attribute__((address_space(256))) int &' and '__attribute__((address_space(256))) MyInt1 &' is '__attribute__((address_space(256))) int &' +// CHECK-MESSAGES: :[[@LINE-5]]:5: note: 'const __attribute__((address_space(256))) int &' and '__attribute__((address_space(256))) MyInt1 &' parameters accept and bind the same kind of values + void attributedParam2(__attribute__((address_space(256))) int *One, const __attribute__((address_space(256))) MyInt1 *Two) {} // CHECK-MESSAGES: :[[@LINE-2]]:23: warning: 2 adjacent parameters of 'attributedParam2' of similar type are // CHECK-MESSAGES: :[[@LINE-3]]:64: note: the first parameter in the range is 'One' // CHECK-MESSAGES: :[[@LINE-3]]:73: note: the last parameter in the range is 'Two' -// CHECK-MESSAGES: :[[@LINE-5]]:23: note: after resolving type aliases, the common type of '__attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' is 'int' +// CHECK-MESSAGES: :[[@LINE-5]]:23: note: after resolving type aliases, '__attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' share a common type // CHECK-MESSAGES: :[[@LINE-5]]:23: note: '__attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' parameters accept and bind the same kind of values -// FIXME: The last diagnostic line is a bit bad: the common type should be a -// pointer type -- it is not clear right now, how it would be possible to -// properly wire a logic in that fixes it.