[clang-tools-extra] [clang-tidy] Speed up/rewrite `bugprone-stringview-nullptr` (PR #192889)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Apr 19 21:34:02 PDT 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
Author: Victor Chernyakin (localspook)
<details>
<summary>Changes</summary>
First, the main attraction: this change makes this quite expensive check basically free:
```cpp
---User Time--- --System Time-- --User+System-- ---Wall Time--- --- Name ---
Before: 0.6406 ( 1.2%) 0.1094 ( 1.7%) 0.7500 ( 1.3%) 0.7482 ( 1.3%) bugprone-stringview-nullptr
After: 0.0000 ( 0.0%) 0.0156 ( 0.3%) 0.0156 ( 0.0%) 0.0028 ( 0.0%) bugprone-stringview-nullptr
```
Now, the details:
- The check now uses the same mechanism as `-Wnonnull`, which is more powerful (before, it couldn't even flag `std::string_view {0}`, and the docs recommended running `modernize-use-nullptr` beforehand).
- The warning message is a bit less detailed.
- The fix-it is slightly worse in certain cases. For example, I've removed the logic that turns `sv == nullptr` into `sv.empty()` (it now becomes just `sv == ""`). My reasoning is: the fix-it logic makes up 90% of the code, but it only provides a small part of the value (detecting UB is the important part), so it's better to have the simpler code.
---
Sorry for the big patch, I wasn't sure how to split it up. However, it might be easier to review than it first seems:
- The code itself is probably easiest to review as an entirely new check. This is ~100 LOC.
- The tests also only have ~100 interesting LOC, and I've left comments pointing them out. The rest are just due to the changed warning message.
---
Patch is 100.97 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/192889.diff
5 Files Affected:
- (modified) clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp (+61-262)
- (modified) clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.h (+5-13)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
- (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/stringview-nullptr.rst (-4)
- (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/stringview-nullptr.cpp (+318-396)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
index 6da20d3f6fecb..b6cc9c0853402 100644
--- a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.cpp
@@ -7,294 +7,93 @@
//===----------------------------------------------------------------------===//
#include "StringviewNullptrCheck.h"
-#include "../utils/TransformerClangTidyCheck.h"
-#include "clang/AST/Decl.h"
-#include "clang/AST/OperationKinds.h"
+#include "../utils/LexerUtils.h"
#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Tooling/Transformer/RangeSelector.h"
-#include "clang/Tooling/Transformer/RewriteRule.h"
-#include "clang/Tooling/Transformer/Stencil.h"
-#include "llvm/ADT/StringRef.h"
-
-namespace clang::tidy::bugprone {
using namespace ::clang::ast_matchers;
-using namespace ::clang::transformer;
+
+namespace clang::tidy::bugprone {
namespace {
-AST_MATCHER_P(InitListExpr, initCountIs, unsigned, N) {
- return Node.getNumInits() == N;
-}
+AST_MATCHER(CXXConstructExpr, constructedFromNullptr) {
+ if (Node.getNumArgs() != 1)
+ return false;
-AST_MATCHER(VarDecl, isDirectInitialization) {
- return Node.getInitStyle() != VarDecl::InitializationStyle::CInit;
+ const Expr *Arg = Node.getArg(0);
+ bool ArgValue; // NOLINT(cppcoreguidelines-init-variables)
+ return !Arg->isValueDependent() &&
+ Arg->EvaluateAsBooleanCondition(ArgValue, Finder->getASTContext()) &&
+ !ArgValue;
}
} // namespace
-static RewriteRuleWith<std::string> stringviewNullptrCheckImpl() {
- auto ConstructionWarning =
- cat("constructing basic_string_view from null is undefined; replace with "
- "the default constructor");
- auto StaticCastWarning =
- cat("casting to basic_string_view from null is undefined; replace with "
- "the empty string");
- auto ArgumentConstructionWarning =
- cat("passing null as basic_string_view is undefined; replace with the "
- "empty string");
- auto AssignmentWarning =
- cat("assignment to basic_string_view from null is undefined; replace "
- "with the default constructor");
- auto RelativeComparisonWarning =
- cat("comparing basic_string_view to null is undefined; replace with the "
- "empty string");
- auto EqualityComparisonWarning =
- cat("comparing basic_string_view to null is undefined; replace with the "
- "emptiness query");
-
- // Matches declarations and expressions of type `basic_string_view`
- auto HasBasicStringViewType = hasType(hasUnqualifiedDesugaredType(recordType(
- hasDeclaration(cxxRecordDecl(hasName("::std::basic_string_view"))))));
-
- // Matches `nullptr` and `(nullptr)` binding to a pointer
- auto NullLiteral = implicitCastExpr(
- hasCastKind(CK_NullToPointer),
- hasSourceExpression(ignoringParens(cxxNullPtrLiteralExpr())));
-
- // Matches `{nullptr}` and `{(nullptr)}` binding to a pointer
- auto NullInitList = initListExpr(initCountIs(1), hasInit(0, NullLiteral));
-
- // Matches `{}`
- auto EmptyInitList = initListExpr(initCountIs(0));
-
- // Matches null construction without `basic_string_view` type spelling
- auto BasicStringViewConstructingFromNullExpr =
- cxxConstructExpr(
- HasBasicStringViewType, argumentCountIs(1),
- hasAnyArgument(/* `hasArgument` would skip over parens */ anyOf(
- NullLiteral, NullInitList, EmptyInitList)),
- unless(cxxTemporaryObjectExpr(/* filters out type spellings */)),
- has(expr().bind("null_arg_expr")))
- .bind("construct_expr");
-
- // `std::string_view(null_arg_expr)`
- auto HandleTemporaryCXXFunctionalCastExpr =
- makeRule(cxxFunctionalCastExpr(hasSourceExpression(
- BasicStringViewConstructingFromNullExpr)),
- remove(node("null_arg_expr")), ConstructionWarning);
+void StringviewNullptrCheck::registerMatchers(MatchFinder *Finder) {
+ const auto HasBasicStringViewType =
+ hasType(hasUnqualifiedDesugaredType(recordType(
+ hasDeclaration(cxxRecordDecl(hasName("::std::basic_string_view"))))));
- // `std::string_view{null_arg_expr}` and `(std::string_view){null_arg_expr}`
- auto HandleTemporaryCXXTemporaryObjectExprAndCompoundLiteralExpr = makeRule(
- cxxTemporaryObjectExpr(cxxConstructExpr(
- HasBasicStringViewType, argumentCountIs(1),
- hasAnyArgument(/* `hasArgument` would skip over parens */ anyOf(
- NullLiteral, NullInitList, EmptyInitList)),
- has(expr().bind("null_arg_expr")))),
- remove(node("null_arg_expr")), ConstructionWarning);
-
- // `(std::string_view) null_arg_expr`
- auto HandleTemporaryCStyleCastExpr =
- makeRule(cStyleCastExpr(hasSourceExpression(
- BasicStringViewConstructingFromNullExpr)),
- changeTo(node("null_arg_expr"), cat("{}")), ConstructionWarning);
-
- // `static_cast<std::string_view>(null_arg_expr)`
- auto HandleTemporaryCXXStaticCastExpr =
- makeRule(cxxStaticCastExpr(hasSourceExpression(
- BasicStringViewConstructingFromNullExpr)),
- changeTo(node("null_arg_expr"), cat("\"\"")), StaticCastWarning);
-
- // `std::string_view sv = null_arg_expr;`
- auto HandleStackCopyInitialization =
- makeRule(varDecl(HasBasicStringViewType,
- hasInitializer(ignoringImpCasts(cxxConstructExpr(
- BasicStringViewConstructingFromNullExpr,
- unless(isListInitialization())))),
- unless(isDirectInitialization())),
- changeTo(node("null_arg_expr"), cat("{}")), ConstructionWarning);
-
- // `std::string_view sv = {null_arg_expr};`
- auto HandleStackCopyListInitialization =
- makeRule(varDecl(HasBasicStringViewType,
- hasInitializer(cxxConstructExpr(
- BasicStringViewConstructingFromNullExpr,
- isListInitialization())),
- unless(isDirectInitialization())),
- remove(node("null_arg_expr")), ConstructionWarning);
-
- // `std::string_view sv(null_arg_expr);`
- auto HandleStackDirectInitialization =
- makeRule(varDecl(HasBasicStringViewType,
- hasInitializer(cxxConstructExpr(
- BasicStringViewConstructingFromNullExpr,
- unless(isListInitialization()))),
- isDirectInitialization())
- .bind("var_decl"),
- changeTo(node("construct_expr"), cat(name("var_decl"))),
- ConstructionWarning);
-
- // `std::string_view sv{null_arg_expr};`
- auto HandleStackDirectListInitialization =
- makeRule(varDecl(HasBasicStringViewType,
- hasInitializer(cxxConstructExpr(
- BasicStringViewConstructingFromNullExpr,
- isListInitialization())),
- isDirectInitialization()),
- remove(node("null_arg_expr")), ConstructionWarning);
-
- // `struct S { std::string_view sv = null_arg_expr; };`
- auto HandleFieldInClassCopyInitialization = makeRule(
- fieldDecl(HasBasicStringViewType,
- hasInClassInitializer(ignoringImpCasts(
- cxxConstructExpr(BasicStringViewConstructingFromNullExpr,
- unless(isListInitialization()))))),
- changeTo(node("null_arg_expr"), cat("{}")), ConstructionWarning);
-
- // `struct S { std::string_view sv = {null_arg_expr}; };` and
- // `struct S { std::string_view sv{null_arg_expr}; };`
- auto HandleFieldInClassCopyListAndDirectListInitialization = makeRule(
- fieldDecl(HasBasicStringViewType,
- hasInClassInitializer(ignoringImpCasts(
- cxxConstructExpr(BasicStringViewConstructingFromNullExpr,
- isListInitialization())))),
- remove(node("null_arg_expr")), ConstructionWarning);
+ Finder->addMatcher(
+ cxxConstructExpr(HasBasicStringViewType, constructedFromNullptr())
+ .bind("construct_expr"),
+ this);
+}
- // `class C { std::string_view sv; C() : sv(null_arg_expr) {} };`
- auto HandleConstructorDirectInitialization =
- makeRule(cxxCtorInitializer(forField(fieldDecl(HasBasicStringViewType)),
- withInitializer(cxxConstructExpr(
- BasicStringViewConstructingFromNullExpr,
- unless(isListInitialization())))),
- remove(node("null_arg_expr")), ConstructionWarning);
+void StringviewNullptrCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *ConstructExpr =
+ Result.Nodes.getNodeAs<CXXConstructExpr>("construct_expr");
+ const Expr *NullArg = ConstructExpr->getArg(0);
- // `class C { std::string_view sv; C() : sv{null_arg_expr} {} };`
- auto HandleConstructorDirectListInitialization =
- makeRule(cxxCtorInitializer(forField(fieldDecl(HasBasicStringViewType)),
- withInitializer(cxxConstructExpr(
- BasicStringViewConstructingFromNullExpr,
- isListInitialization()))),
- remove(node("null_arg_expr")), ConstructionWarning);
+ auto Diag = diag(NullArg->getBeginLoc(),
+ "constructing basic_string_view from null is undefined");
- // `void f(std::string_view sv = null_arg_expr);`
- auto HandleDefaultArgumentCopyInitialization =
- makeRule(parmVarDecl(HasBasicStringViewType,
- hasInitializer(ignoringImpCasts(cxxConstructExpr(
- BasicStringViewConstructingFromNullExpr,
- unless(isListInitialization()))))),
- changeTo(node("null_arg_expr"), cat("{}")), ConstructionWarning);
+ const std::optional<Token> PrevToken = utils::lexer::getPreviousToken(
+ NullArg->getBeginLoc(), *Result.SourceManager, getLangOpts());
- // `void f(std::string_view sv = {null_arg_expr});`
- auto HandleDefaultArgumentCopyListInitialization =
- makeRule(parmVarDecl(HasBasicStringViewType,
- hasInitializer(cxxConstructExpr(
- BasicStringViewConstructingFromNullExpr,
- isListInitialization()))),
- remove(node("null_arg_expr")), ConstructionWarning);
+ if (!PrevToken)
+ return;
- // `new std::string_view(null_arg_expr)`
- auto HandleHeapDirectInitialization = makeRule(
- cxxNewExpr(has(cxxConstructExpr(BasicStringViewConstructingFromNullExpr,
- unless(isListInitialization()))),
- unless(isArray()), unless(hasAnyPlacementArg(anything()))),
- remove(node("null_arg_expr")), ConstructionWarning);
+ const auto NullArgReplacement = [&]() -> StringRef {
+ // 'sv = nullptr;' -> 'sv = {};'
+ // '(std::string_view)nullptr' -> '(std::string_view){}'
+ // 'return nullptr;' -> 'return {};'
+ if (PrevToken->isOneOf(tok::equal, tok::r_paren) ||
+ (PrevToken->is(tok::raw_identifier) &&
+ PrevToken->getRawIdentifier() == "return"))
+ return "{}";
- // `new std::string_view{null_arg_expr}`
- auto HandleHeapDirectListInitialization = makeRule(
- cxxNewExpr(has(cxxConstructExpr(BasicStringViewConstructingFromNullExpr,
- isListInitialization())),
- unless(isArray()), unless(hasAnyPlacementArg(anything()))),
- remove(node("null_arg_expr")), ConstructionWarning);
+ // Implicit constructor call.
+ if (ConstructExpr->getSourceRange() == NullArg->getSourceRange())
+ return "\"\"";
- // `function(null_arg_expr)`
- auto HandleFunctionArgumentInitialization =
- makeRule(callExpr(hasAnyArgument(ignoringImpCasts(
- BasicStringViewConstructingFromNullExpr)),
- unless(cxxOperatorCallExpr())),
- changeTo(node("construct_expr"), cat("\"\"")),
- ArgumentConstructionWarning);
+ if (PrevToken->is(tok::l_brace))
+ return {};
- // `sv = null_arg_expr`
- auto HandleAssignment = makeRule(
- cxxOperatorCallExpr(hasOverloadedOperatorName("="),
- hasRHS(materializeTemporaryExpr(
- has(BasicStringViewConstructingFromNullExpr)))),
- changeTo(node("construct_expr"), cat("{}")), AssignmentWarning);
+ if (PrevToken->is(tok::l_paren)) {
+ const DynTypedNodeList Parents =
+ Result.Context->getParentMapContext().getParents(*ConstructExpr);
- // `sv < null_arg_expr`
- auto HandleRelativeComparison = makeRule(
- cxxOperatorCallExpr(hasAnyOverloadedOperatorName("<", "<=", ">", ">="),
- hasEitherOperand(ignoringImpCasts(
- BasicStringViewConstructingFromNullExpr))),
- changeTo(node("construct_expr"), cat("\"\"")), RelativeComparisonWarning);
+ if (Parents.empty())
+ return {};
- // `sv == null_arg_expr`
- auto HandleEmptyEqualityComparison = makeRule(
- cxxOperatorCallExpr(
- hasOverloadedOperatorName("=="),
- hasOperands(ignoringImpCasts(BasicStringViewConstructingFromNullExpr),
- traverse(TK_IgnoreUnlessSpelledInSource,
- expr().bind("instance"))))
- .bind("root"),
- changeTo(node("root"), cat(access("instance", cat("empty")), "()")),
- EqualityComparisonWarning);
+ if (Parents[0].get<CXXStaticCastExpr>())
+ return "\"\"";
- // `sv != null_arg_expr`
- auto HandleNonEmptyEqualityComparison = makeRule(
- cxxOperatorCallExpr(
- hasOverloadedOperatorName("!="),
- hasOperands(ignoringImpCasts(BasicStringViewConstructingFromNullExpr),
- traverse(TK_IgnoreUnlessSpelledInSource,
- expr().bind("instance"))))
- .bind("root"),
- changeTo(node("root"), cat("!", access("instance", cat("empty")), "()")),
- EqualityComparisonWarning);
+ // Avoid causing a most vexing parse.
+ if (const auto *Var = Parents[0].get<VarDecl>())
+ if (Var->getInitStyle() == VarDecl::InitializationStyle::CallInit)
+ Diag << FixItHint::CreateRemoval(
+ ConstructExpr->getParenOrBraceRange());
- // `return null_arg_expr;`
- auto HandleReturnStatement = makeRule(
- returnStmt(hasReturnValue(
- ignoringImpCasts(BasicStringViewConstructingFromNullExpr))),
- changeTo(node("construct_expr"), cat("{}")), ConstructionWarning);
+ return {};
+ }
- // `T(null_arg_expr)`
- auto HandleConstructorInvocation =
- makeRule(cxxConstructExpr(
- hasAnyArgument(/* `hasArgument` would skip over parens */
- ignoringImpCasts(
- BasicStringViewConstructingFromNullExpr)),
- unless(HasBasicStringViewType)),
- changeTo(node("construct_expr"), cat("\"\"")),
- ArgumentConstructionWarning);
+ return "\"\"";
+ }();
- return applyFirst(
- {HandleTemporaryCXXFunctionalCastExpr,
- HandleTemporaryCXXTemporaryObjectExprAndCompoundLiteralExpr,
- HandleTemporaryCStyleCastExpr,
- HandleTemporaryCXXStaticCastExpr,
- HandleStackCopyInitialization,
- HandleStackCopyListInitialization,
- HandleStackDirectInitialization,
- HandleStackDirectListInitialization,
- HandleFieldInClassCopyInitialization,
- HandleFieldInClassCopyListAndDirectListInitialization,
- HandleConstructorDirectInitialization,
- HandleConstructorDirectListInitialization,
- HandleDefaultArgumentCopyInitialization,
- HandleDefaultArgumentCopyListInitialization,
- HandleHeapDirectInitialization,
- HandleHeapDirectListInitialization,
- HandleFunctionArgumentInitialization,
- HandleAssignment,
- HandleRelativeComparison,
- HandleEmptyEqualityComparison,
- HandleNonEmptyEqualityComparison,
- HandleReturnStatement,
- HandleConstructorInvocation});
+ Diag << FixItHint::CreateReplacement(NullArg->getSourceRange(),
+ NullArgReplacement);
}
-StringviewNullptrCheck::StringviewNullptrCheck(StringRef Name,
- ClangTidyContext *Context)
- : utils::TransformerClangTidyCheck(stringviewNullptrCheckImpl(), Name,
- Context) {}
-
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.h b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.h
index 183a4f776a7f6..6d78764d83cf5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/StringviewNullptrCheck.h
@@ -9,7 +9,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRINGVIEWNULLPTRCHECK_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_STRINGVIEWNULLPTRCHECK_H
-#include "../utils/TransformerClangTidyCheck.h"
+#include "../ClangTidyCheck.h"
namespace clang::tidy::bugprone {
@@ -19,21 +19,13 @@ namespace clang::tidy::bugprone {
/// braced initializer list does not compile so instead a call to `.empty()` or
/// the empty string literal are used, where appropriate.
///
-/// This prevents code from invoking behavior which is unconditionally
-/// undefined. The single-argument `const CharT*` constructor does not check
-/// for the null case before dereferencing its input. The standard is slated to
-/// add an explicitly-deleted overload to catch some of these cases:
-/// wg21.link/p2166
-///
-/// To catch the additional cases of `NULL` (which expands to `__null`) and
-/// `0`, first run the ``modernize-use-nullptr`` check to convert the callers
-/// to `nullptr`.
-///
/// For the user-facing documentation see:
/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/stringview-nullptr.html
-class StringviewNullptrCheck : public utils::TransformerClangTidyCheck {
+class StringviewNullptrCheck : public ClangTidyCheck {
public:
- StringviewNullptrCheck(StringRef Name, ClangTidyContext *Context);
+ using ClangTidyCheck::ClangTidyCheck;
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus17;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 95ed0061d654c..5ca5769121d61 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -291,6 +291,10 @@ Changes in existing checks
string constructor calls when the string class constructor has a default
allocator argument.
+- Improved :doc:`bugprone-stringview-nullptr
+ <clang-tidy/checks/bugprone/stringview-nullptr>` to flag more subtle
+ cases where a string view is initialized from a ``nullptr``.
+
- Improved :doc:`bugprone-throwing-static-initialization
<clang-tidy/checks/bugprone/throwing-static-initialization>` check by adding
the `AllowedTypes` option. With this option it is possible to exclude
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/stringview-nullptr.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/stringview-nullptr.rst
index 7138c97b745ae..d03fb878de9cf 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/stringview-nullptr.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/stringview-nullptr.rst
@@ -13,10 +13,6 @@ The single-argument ``const CharT*`` constructor does not check for the null
case before dereferencing its input. The standard is slated to add an
explicitly-deleted overload to catch some of these cases: wg21.link/p2166
-To catch the additional cases of ``NULL`` (which expands to ``__null``) and
-``0``, first run the ``modernize-use-nullptr`` check to convert the callers to
-``nullptr``.
-
.. code-block:: c++
std::stri...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/192889
More information about the cfe-commits
mailing list