whisperity updated this revision to Diff 360269.
whisperity marked 4 inline comments as done.
whisperity added a comment.
- Remove orthogonal/half-baked changes.
- Fix a mistake in the logic changing only a temporary state.
- Add a missing //FIXME//.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106361/new/
https://reviews.llvm.org/D106361
Files:
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
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -113,3 +113,14 @@
// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'Dest'
// 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 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: '__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 core 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.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.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-len2.cpp
@@ -346,3 +346,31 @@
void functionPrototypeLosesNoexcept(void (*NonThrowing)() noexcept, void (*Throwing)()) {}
// NO-WARN: This call cannot be swapped, even if "getCanonicalType()" believes otherwise.
+
+void attributedParam1(const __attribute__((address_space(256))) int *One,
+ const __attribute__((address_space(256))) int *Two) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:23: warning: 2 adjacent parameters of 'attributedParam1' of similar type ('const __attribute__((address_space(256))) int *') are
+// CHECK-MESSAGES: :[[@LINE-3]]:70: note: the first parameter in the range is 'One'
+// CHECK-MESSAGES: :[[@LINE-3]]:70: note: the last parameter in the range is 'Two'
+
+void attributedParam1Typedef(const __attribute__((address_space(256))) int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// 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 core 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.
+
+void attributedParam2(__attribute__((address_space(256))) int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// NO-WARN: One is CVR-qualified, the other is not.
+
+void attributedParam3(const int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// NO-WARN: One is attributed, the other is not.
+
+void attributedParam4(const __attribute__((address_space(512))) int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// NO-WARN: Different value of the attribute.
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -470,17 +470,18 @@
}
/// Add the specified qualifiers to the common type in the Mix.
- MixData qualify(Qualifiers Quals) const {
- SplitQualType Split = CommonType.split();
- Split.Quals.addQualifiers(Quals);
- QualType CommonType{Split.Ty, Split.Quals.getAsOpaqueValue()};
+ MixData qualify(Qualifiers Quals, const ASTContext &Ctx) const {
+ if (CommonType.isNull())
+ return *this;
+
+ QualType NewCommonType = Ctx.getQualifiedType(CommonType, Quals);
if (CreatedFromOneWayConversion) {
MixData M{Flags, Conversion};
- M.CommonType = CommonType;
+ M.CommonType = NewCommonType;
return M;
}
- return {Flags, CommonType, Conversion, ConversionRTL};
+ return {Flags, NewCommonType, Conversion, ConversionRTL};
}
};
@@ -566,7 +567,41 @@
ImplicitConversionModellingMode ImplicitMode);
static inline bool isUselessSugar(const Type *T) {
- return isa<DecayedType, ElaboratedType, ParenType>(T);
+ return isa<AttributedType, DecayedType, ElaboratedType, ParenType>(T);
+}
+
+/// 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';);
+ Qualifiers LQual = LType.getLocalQualifiers(),
+ RQual = RType.getLocalQualifiers();
+
+ // Strip potential CVR. That is handled by the check option QualifiersMix.
+ LQual.removeCVRQualifiers();
+ RQual.removeCVRQualifiers();
+
+ Qualifiers CommonQuals = Qualifiers::removeCommonQualifiers(LQual, RQual);
+ (void)CommonQuals;
+
+ LLVM_DEBUG(llvm::dbgs() << "--- hasNonCVRMixabilityBreakingQualifiers. "
+ "Removed common qualifiers: ";
+ CommonQuals.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 all other qualifiers are common between the two types, good to go.
+ return LQual.hasQualifiers() || RQual.hasQualifiers();
}
/// Approximate the way how LType and RType might refer to "essentially the
@@ -585,12 +620,6 @@
LLVM_DEBUG(llvm::dbgs() << ">>> calculateMixability for LType:\n";
LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n";
RType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';);
-
- // Certain constructs match on the last catch-all getCanonicalType() equality,
- // which is perhaps something not what we want. If this variable is true,
- // the canonical type equality will be ignored.
- bool RecursiveReturnDiscardingCanonicalType = false;
-
if (LType == RType) {
LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Trivial equality.\n");
return {MixFlags::Trivial, LType};
@@ -628,7 +657,6 @@
MixFlags::ReferenceBind;
}
- // Dissolve typedefs after the qualifiers outside the typedef are dealt with.
if (LType->getAs<TypedefType>()) {
LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is typedef.\n");
return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx),
@@ -645,35 +673,72 @@
// A parameter of type 'cvr1 T' and another of potentially differently
// qualified 'cvr2 T' may bind with the same power, if the user so requested.
+ //
+ // 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 is CVR.\n");
- LLVM_DEBUG(if (RType.getLocalCVRQualifiers()) llvm::dbgs()
- << "--- calculateMixability. RHS is CVR.\n");
+ LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) {
+ llvm::dbgs() << "--- calculateMixability. LHS has CVR-Qualifiers: ";
+ Qualifiers::fromCVRMask(LType.getLocalCVRQualifiers())
+ .print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << '\n';
+ });
+ LLVM_DEBUG(if (RType.getLocalCVRQualifiers()) {
+ llvm::dbgs() << "--- calculateMixability. RHS has CVR-Qualifiers: ";
+ Qualifiers::fromCVRMask(RType.getLocalCVRQualifiers())
+ .print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << '\n';
+ });
if (!Check.QualifiersMix) {
LLVM_DEBUG(llvm::dbgs()
- << "<<< calculateMixability. QualifiersMix turned off.\n");
+ << "<<< calculateMixability. QualifiersMix turned off - not "
+ "mixable.\n");
return {MixFlags::None};
}
- return calculateMixability(Check, LType.getLocalUnqualifiedType(),
- RType.getLocalUnqualifiedType(), Ctx,
- ImplicitMode) |
- MixFlags::Qualifiers;
+ CompareUnqualifiedTypes = true;
}
if (LType.getLocalCVRQualifiers() == RType.getLocalCVRQualifiers() &&
LType.getLocalCVRQualifiers() != 0) {
- LLVM_DEBUG(llvm::dbgs()
- << "--- calculateMixability. LHS and RHS same CVR.\n");
- // Apply the same qualifier back into the found common type if we found
- // a common type between the unqualified versions.
- return calculateMixability(Check, LType.getLocalUnqualifiedType(),
- RType.getLocalUnqualifiedType(), Ctx,
- ImplicitMode)
- .qualify(LType.getLocalQualifiers());
+ LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) {
+ llvm::dbgs()
+ << "--- calculateMixability. LHS and RHS have same CVR-Qualifiers: ";
+ Qualifiers::fromCVRMask(LType.getLocalCVRQualifiers())
+ .print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << '\n';
+ });
+
+ CompareUnqualifiedTypes = true;
+ RequalifyUnqualifiedMixabilityResult = true;
+ }
+
+ if (CompareUnqualifiedTypes) {
+ if (hasNonCVRMixabilityBreakingQualifiers(Ctx, LType, RType)) {
+ LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Additional "
+ "non-equal incompatible qualifiers.\n");
+ return {MixFlags::None};
+ }
+
+ MixData UnqualifiedMixability =
+ calculateMixability(Check, LType.getLocalUnqualifiedType(),
+ RType.getLocalUnqualifiedType(), Ctx, ImplicitMode);
+
+ if (!RequalifyUnqualifiedMixabilityResult)
+ return UnqualifiedMixability | MixFlags::Qualifiers;
+
+ // Apply the same qualifier back into the found core type if we found
+ // a core type between the unqualified versions.
+ return UnqualifiedMixability.qualify(LType.getLocalQualifiers(), Ctx);
}
+ // Certain constructs match on the last catch-all getCanonicalType() equality,
+ // which is perhaps something not what we want. If this variable is true,
+ // the canonical type equality will be ignored.
+ bool RecursiveReturnDiscardingCanonicalType = false;
+
if (LType->isPointerType() && RType->isPointerType()) {
// If both types are pointers, and pointed to the exact same type,
// LType == RType took care of that. Try to see if the pointee type has
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
