llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Andrew Gaul (gaul) <details> <summary>Changes</summary> The check rewrites ``` v.push_back(T(42, 0)); ``` where the second parameter of the constructor of `T` is a pointer, to `v.emplace_back(42, 0)`. The forwarding reference of `emplace_back` deduces `0` as an `int`, which, unlike the literal, does not convert to a pointer, so the rewritten call does not compile. `NULL`, which is `__null` or an integer literal, has the same problem, as does a literal zero passed for a pointer to member. Ignore such calls, as the check already does for bit-field arguments, and remove the FIXME about them. `nullptr` is deduced as `std::nullptr_t`, which converts to any pointer, so calls passing it are still reported. --- Full diff: https://github.com/llvm/llvm-project/pull/237770.diff 4 Files Affected: - (modified) clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp (+12-4) - (modified) clang-tools-extra/docs/ReleaseNotes.md (+5) - (modified) clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst (+3) - (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp (+31) ``````````diff diff --git a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp index 8ec123fc476ff4..ea033e0794a995 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp @@ -181,6 +181,13 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { ignoringImplicit(allOf(cxxConstructExpr(isListInitialization()), unless(cxxTemporaryObjectExpr())))); + // 0 and NULL convert to null pointers, but emplace_back would deduce them as + // integers, which don't. + auto NullPointerConstantAsArgument = hasAnyArgument( + expr(hasType(hasCanonicalType(anyOf(pointerType(), memberPointerType()))), + ignoringImplicit( + ignoringParenImpCasts(anyOf(integerLiteral(), gnuNullExpr()))))); + // We could have leak of resource. auto NewExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr())); // We would call another constructor. @@ -194,13 +201,14 @@ void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) { auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())), has(cxxStdInitializerListExpr())); - // FIXME: Discard 0/NULL (as nullptr), static inline const data members, - // overloaded functions and template names. + // FIXME: Discard static inline const data members, overloaded functions and + // template names. const auto SoughtConstructExpr = cxxConstructExpr( unless(anyOf(IsCtorOfSmartPtr, HasInitList, BitFieldAsArgument, - InitializerListAsArgument, NewExprAsArgument, - ConstructingDerived, IsPrivateOrProtectedCtor))) + NullPointerConstantAsArgument, InitializerListAsArgument, + NewExprAsArgument, ConstructingDerived, + IsPrivateOrProtectedCtor))) .bind("ctor"); auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr)); diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 679ca9ecbb4a39..4cff278bdb91f8 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -301,6 +301,11 @@ infrastructure are described first, followed by tool-specific sections. <clang-tidy/checks/modernize/use-designated-initializers>` when analyzing malformed code with nested classes and ambiguous initializer. +- Fixed false positive in {doc}`modernize-use-emplace + <clang-tidy/checks/modernize/use-emplace>` check when a constructor argument + is `0` or `NULL` used as a null pointer, which an `emplace`-like method would + forward as an integer. + - Improved {doc}`modernize-use-equals-default <clang-tidy/checks/modernize/use-equals-default>` check to also capture default constructor with explicit default-initialization of parents. diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst index f1031f69a94679..bdcd6126298a83 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-emplace.rst @@ -111,6 +111,9 @@ Check also doesn't fire if any argument of the constructor call would be: - a bit-field (bit-fields can't bind to rvalue/universal reference) + - ``0`` or ``NULL`` converted to a null pointer (a universal reference + deduces them as integers, which don't convert to pointers) + - a ``new`` expression (to avoid leak) - if the argument would be converted via derived-to-base cast. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp index a2ab19efcf6981..ae920f991c39aa 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-emplace.cpp @@ -196,6 +196,37 @@ void testNewExpr() { v.push_back(Derived(new int, Something{})); } +struct WithMember { + int Member; +}; + +struct MemberPointer { + MemberPointer(int WithMember::*) {} +}; + +void testNullPointerConstants() { + std::vector<Base> v; + v.push_back(Base(42, 0)); + v.push_back(Base(42, (0))); + v.push_back(Base(42, 0L)); + v.push_back(Base(42, __null)); + + v.push_back(Base(42, nullptr)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(42, nullptr); + + v.push_back(Base(0, nullptr, 0)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: v.emplace_back(0, nullptr, 0); + + std::vector<MemberPointer> w; + w.push_back(MemberPointer(0)); + + w.push_back(MemberPointer(nullptr)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back + // CHECK-FIXES: w.emplace_back(nullptr); +} + void testSpaces() { std::vector<Something> v; `````````` </details> https://github.com/llvm/llvm-project/pull/237770 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
