PiotrZSL created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
PiotrZSL added a comment.
Eugene.Zelenko added reviewers: aaron.ballman, carlosgalvezp.
PiotrZSL updated this revision to Diff 498450.
PiotrZSL published this revision for review.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
I may consider tomorrow moving `std::array` into into some dedicated option,
like IgnoredContainersInComparisonRegexp
PiotrZSL added a comment.
Add configuration option. Release Notes + documentation for new option powered
by ChatGPT.
PiotrZSL added a comment.
Ready for review
Ignoring std::array type when matching 'std:array == std::array()'.
In such case we shouldn't propose to use empty().
Fixes: https://github.com/llvm/llvm-project/issues/48286
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D144217
Files:
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp
@@ -724,10 +724,37 @@
size_type size() const;
bool empty() const;
};
-void test() {
+
+void testTypedefSize() {
TypedefSize ts;
if (ts.size() == 0)
;
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: the 'empty' method should be used
// CHECK-FIXES: {{^ }}if (ts.empty()){{$}}
}
+
+namespace std {
+
+template <typename T, unsigned long N> struct array {
+ bool operator==(const array& other) const;
+ bool operator!=(const array& other) const;
+ unsigned long size() const { return N; }
+ bool empty() const { return N != 0U; }
+
+ T data[N];
+};
+
+}
+
+typedef std::array<int, 10U> Array;
+
+bool testArraySize(const Array& value) {
+ return value.size() == 0U;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
+// CHECK-FIXES: {{^ }}return value.empty();{{$}}
+// CHECK-MESSAGES: :742:8: note: method 'array'::empty() defined here
+}
+
+bool testArrayCompareToEmptye(const Array& value) {
+ return value == std::array<int, 10U>();
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability/container-size-empty.rst
@@ -24,3 +24,11 @@
bool empty() const;
`size_type` can be any kind of integer type.
+
+.. option:: IgnoreComparisonForTypesRegexp
+
+ Excludes certain classes from the check using a regular expression of ERE
+ syntax. If excluded, the check won't suggest replacing the comparison of
+ an object with a default constructed object of the same type using
+ ``empty()``. This option is useful if the check incorrectly flags such
+ comparisons for certain classes. Default value is `^::std::array`.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -130,6 +130,11 @@
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Fixed a false positive in :doc:`readability-container-size-empty
+ <clang-tidy/checks/readability/container-size-empty>` check when comparing
+ ``std::array`` objects to default constructed ones. The behavior for this and
+ other relevant classes can now be configured with a new option.
+
Removed checks
^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
+++ clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERSIZEEMPTYCHECK_H
#include "../ClangTidyCheck.h"
+#include <string>
namespace clang::tidy::readability {
@@ -31,9 +32,13 @@
}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
std::optional<TraversalKind> getCheckTraversalKind() const override {
return TK_IgnoreUnlessSpelledInSource;
}
+
+private:
+ std::string IgnoreComparisonForTypesRegexp;
};
} // namespace clang::tidy::readability
Index: clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
@@ -96,7 +96,14 @@
ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name,
ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ : ClangTidyCheck(Name, Context),
+ IgnoreComparisonForTypesRegexp(
+ Options.get("IgnoreComparisonForTypesRegexp", "^::std::array")) {}
+
+void ContainerSizeEmptyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "IgnoreComparisonForTypesRegexp",
+ IgnoreComparisonForTypesRegexp);
+}
void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) {
const auto ValidContainerRecord = cxxRecordDecl(isSameOrDerivedFrom(
@@ -164,12 +171,25 @@
hasUnaryOperand(
expr(hasType(pointsTo(ValidContainer))).bind("Pointee"))),
expr(hasType(ValidContainer)).bind("STLObject"));
+
+ const auto StdArrayType = qualType(anyOf(
+ hasDeclaration(cxxRecordDecl(matchesName(IgnoreComparisonForTypesRegexp))
+ .bind("array")),
+ hasCanonicalType(hasDeclaration(
+ cxxRecordDecl(matchesName(IgnoreComparisonForTypesRegexp))
+ .bind("array")))));
+ const auto SameStdArrayType =
+ qualType(anyOf(hasDeclaration(cxxRecordDecl(equalsBoundNode("array"))),
+ hasCanonicalType(hasDeclaration(
+ cxxRecordDecl(equalsBoundNode("array"))))));
+
Finder->addMatcher(
binaryOperation(hasAnyOperatorName("==", "!="),
- hasOperands(WrongComparend,
- STLArg),
- unless(hasAncestor(cxxMethodDecl(
- ofClass(equalsBoundNode("container"))))))
+ hasOperands(WrongComparend, STLArg),
+ unless(allOf(hasLHS(hasType(StdArrayType)),
+ hasRHS(hasType(SameStdArrayType)))),
+ unless(hasAncestor(cxxMethodDecl(
+ ofClass(equalsBoundNode("container"))))))
.bind("BinCmp"),
this);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
