maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[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
This commit is contained in:
Whisperity 2021-07-21 16:09:39 +02:00
parent 6c1f655818
commit 8b0cc4a65d
3 changed files with 102 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

113
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
View File

@ -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);
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 (!RequalifyUnqualifiedMixabilityResult)
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;

21
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
View File

@ -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) {}

14
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
View File

@ -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.