[clang-tools-extra] 8b0cc4a - [clang-tidy] Improve "common type" diagnostic output in 'bugprone-easily-swappable-parameters'
via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 23 01:26:41 PDT 2021
Author: Whisperity
Date: 2021-07-23T10:26:22+02:00
New Revision: 8b0cc4a65dd435096bf64651693f5c9c3e2fee3b
URL: https://github.com/llvm/llvm-project/commit/8b0cc4a65dd435096bf64651693f5c9c3e2fee3b
DIFF: https://github.com/llvm/llvm-project/commit/8b0cc4a65dd435096bf64651693f5c9c3e2fee3b.diff
LOG: [clang-tidy] Improve "common type" diagnostic output in 'bugprone-easily-swappable-parameters'
Make the check handle cases of the "common type" involved in the mix
being non-trivial, e.g. pointers, references, attributes, these things
coming from typedefs, etc.
This results in clearer diagnostics that have more coverage in their
explanation, such as saying `const int &` as common type instead of
`int`.
Reviewed By: aaron.ballman
Differential Revision: http://reviews.llvm.org/D106442
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
index 3cb4734e553b0..afcdca226911d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -469,18 +469,18 @@ struct MixData {
return *this;
}
- /// Add the specified qualifiers to the common type in the Mix.
- MixData qualify(Qualifiers Quals, const ASTContext &Ctx) const {
+ template <class F> 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 @@ struct MixableParameterRange {
/// 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 @@ static inline bool isUselessSugar(const Type *T) {
return isa<AttributedType, DecayedType, ElaboratedType, ParenType>(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 @@ static bool hasNonCVRMixabilityBreakingQualifiers(const ASTContext &Ctx,
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 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
Check, LType, RType.getSingleStepDesugaredType(Ctx), Ctx, ImplicitMode);
}
+ const auto *LLRef = LType->getAs<LValueReferenceType>();
+ const auto *RLRef = RType->getAs<LValueReferenceType>();
+ 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<LValueReferenceType>()) {
+ 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<LValueReferenceType>()) {
+ 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 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
//
// 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 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
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 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
});
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 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
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 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
// 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 @@ void EasilySwappableParametersCheck::check(
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
index 58138c720a399..aeb43e986d537 100644
--- 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 @@ void referenceThroughTypedef(int I, ICRTy Builtin, MyIntCRTy MyInt) {}
// 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 @@ void attributedParam1Typedef(const __attribute__((address_space(256))) int *One,
// 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
index d51f44972dcb8..bf980d951307b 100644
--- 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 @@ void copy(const T *Dest, T *Source) {}
// 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.
More information about the cfe-commits
mailing list