https://github.com/zeyi2 created https://github.com/llvm/llvm-project/pull/171757
Closes #170921 >From 2db4bebc534f2e68fd11945ebf7f9d3812ec2853 Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Wed, 10 Dec 2025 22:34:47 +0800 Subject: [PATCH 01/10] [clang-tidy] Refactor isLikelyTypo to be a template function --- .../bugprone/ArgumentCommentCheck.cpp | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index ed30d01e645d1..65c7865a917cd 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -142,31 +142,46 @@ getCommentsBeforeLoc(ASTContext *Ctx, SourceLocation Loc) { return Comments; } -static bool isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params, - StringRef ArgName, unsigned ArgIndex) { - const std::string ArgNameLowerStr = ArgName.lower(); - const StringRef ArgNameLower = ArgNameLowerStr; +template <typename NamedDeclRange> +static bool isLikelyTypo(const NamedDeclRange &Candidates, StringRef ArgName, + StringRef TargetName) { + llvm::SmallString<64> ArgNameLower; + ArgNameLower.reserve(ArgName.size()); + for (char C : ArgName) + ArgNameLower.push_back(llvm::toLower(C)); + const StringRef ArgNameLowerRef = StringRef(ArgNameLower); // The threshold is arbitrary. const unsigned UpperBound = ((ArgName.size() + 2) / 3) + 1; - const unsigned ThisED = ArgNameLower.edit_distance( - Params[ArgIndex]->getIdentifier()->getName().lower(), + llvm::SmallString<64> TargetNameLower; + TargetNameLower.reserve(TargetName.size()); + for (char C : TargetName) + TargetNameLower.push_back(llvm::toLower(C)); + const unsigned ThisED = ArgNameLowerRef.edit_distance( + StringRef(TargetNameLower), /*AllowReplacements=*/true, UpperBound); if (ThisED >= UpperBound) return false; - for (unsigned I = 0, E = Params.size(); I != E; ++I) { - if (I == ArgIndex) - continue; - const IdentifierInfo *II = Params[I]->getIdentifier(); + for (const auto &Candidate : Candidates) { + const IdentifierInfo *II = Candidate->getIdentifier(); if (!II) continue; + // Skip the target itself. + if (II->getName() == TargetName) + continue; + const unsigned Threshold = 2; - // Other parameters must be an edit distance at least Threshold more away - // from this parameter. This gives us greater confidence that this is a - // typo of this parameter and not one with a similar name. - const unsigned OtherED = ArgNameLower.edit_distance( - II->getName().lower(), + // Other candidates must be an edit distance at least Threshold more away + // from this candidate. This gives us greater confidence that this is a + // typo of this candidate and not one with a similar name. + llvm::SmallString<64> CandidateLower; + StringRef CandName = II->getName(); + CandidateLower.reserve(CandName.size()); + for (char C : CandName) + CandidateLower.push_back(llvm::toLower(C)); + const unsigned OtherED = ArgNameLowerRef.edit_distance( + StringRef(CandidateLower), /*AllowReplacements=*/true, ThisED + Threshold); if (OtherED < ThisED + Threshold) return false; @@ -319,7 +334,7 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, diag(Comment.first, "argument name '%0' in comment does not " "match parameter name %1") << Matches[2] << II; - if (isLikelyTypo(Callee->parameters(), Matches[2], I)) { + if (isLikelyTypo(Callee->parameters(), Matches[2], II->getName())) { Diag << FixItHint::CreateReplacement( Comment.first, (Matches[1] + II->getName() + Matches[3]).str()); } >From 74bd67d178830c5e1d7d6c9c200916b7d9261100 Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Wed, 10 Dec 2025 22:36:25 +0800 Subject: [PATCH 02/10] [clang-tidy] Extract collectLeadingComments helper function --- .../bugprone/ArgumentCommentCheck.cpp | 52 ++++++++++++------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index 65c7865a917cd..1b0527836ac5d 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -258,6 +258,38 @@ static const FunctionDecl *resolveMocks(const FunctionDecl *Func) { return Func; } +// Create a file character range between two locations, or return an invalid +// range if they are from different files or otherwise not representable. +static CharSourceRange makeFileCharRange(ASTContext *Ctx, SourceLocation Begin, + SourceLocation End) { + return Lexer::makeFileCharRange(CharSourceRange::getCharRange(Begin, End), + Ctx->getSourceManager(), + Ctx->getLangOpts()); +} + +// Collects consecutive comments immediately preceding an argument expression. +static llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> +collectLeadingComments(ASTContext *Ctx, SourceLocation SearchBegin, + const Expr *Arg) { + const CharSourceRange BeforeArgument = + makeFileCharRange(Ctx, SearchBegin, Arg->getBeginLoc()); + + if (BeforeArgument.isValid()) { + auto Vec = getCommentsInRange(Ctx, BeforeArgument); + llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> Result; + Result.append(Vec.begin(), Vec.end()); + return Result; + } + + // Fall back to parsing back from the start of the argument. + const CharSourceRange ArgsRange = + makeFileCharRange(Ctx, Arg->getBeginLoc(), Arg->getEndLoc()); + auto Vec = getCommentsBeforeLoc(Ctx, ArgsRange.getBegin()); + llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> Result; + Result.append(Vec.begin(), Vec.end()); + return Result; +} + // Given the argument type and the options determine if we should // be adding an argument comment. bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const { @@ -289,12 +321,6 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, if ((NumArgs == 0) || (IgnoreSingleArgument && NumArgs == 1)) return; - auto MakeFileCharRange = [Ctx](SourceLocation Begin, SourceLocation End) { - return Lexer::makeFileCharRange(CharSourceRange::getCharRange(Begin, End), - Ctx->getSourceManager(), - Ctx->getLangOpts()); - }; - for (unsigned I = 0; I < NumArgs; ++I) { const ParmVarDecl *PVD = Callee->getParamDecl(I); const IdentifierInfo *II = PVD->getIdentifier(); @@ -311,20 +337,10 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, } } - const CharSourceRange BeforeArgument = - MakeFileCharRange(ArgBeginLoc, Args[I]->getBeginLoc()); + llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> Comments = + collectLeadingComments(Ctx, ArgBeginLoc, Args[I]); ArgBeginLoc = Args[I]->getEndLoc(); - std::vector<std::pair<SourceLocation, StringRef>> Comments; - if (BeforeArgument.isValid()) { - Comments = getCommentsInRange(Ctx, BeforeArgument); - } else { - // Fall back to parsing back from the start of the argument. - const CharSourceRange ArgsRange = - MakeFileCharRange(Args[I]->getBeginLoc(), Args[I]->getEndLoc()); - Comments = getCommentsBeforeLoc(Ctx, ArgsRange.getBegin()); - } - for (auto Comment : Comments) { llvm::SmallVector<StringRef, 2> Matches; if (IdentRE.match(Comment.second, &Matches) && >From 048ca1e0cbc98dbbec43b49a185935155057b12f Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Wed, 10 Dec 2025 22:36:53 +0800 Subject: [PATCH 03/10] [clang-tidy] Extract diagnoseMismatchInComment helper function --- .../bugprone/ArgumentCommentCheck.cpp | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index 1b0527836ac5d..41b571f46dfdc 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -290,6 +290,32 @@ collectLeadingComments(ASTContext *Ctx, SourceLocation SearchBegin, return Result; } +template <typename NamedDeclRange> +static bool diagnoseMismatchInComment( + ArgumentCommentCheck &Self, ASTContext *Ctx, + const NamedDeclRange &Candidates, const IdentifierInfo *II, + bool StrictMode, llvm::Regex &IdentRE, + const std::pair<SourceLocation, StringRef> &Comment, + SourceLocation DeclLoc) { + llvm::SmallVector<StringRef, 2> Matches; + if (!(IdentRE.match(Comment.second, &Matches) && + !sameName(Matches[2], II->getName(), StrictMode))) + return false; + + { + const DiagnosticBuilder Diag = Self.diag( + Comment.first, + "argument name '%0' in comment does not match parameter name %1") + << Matches[2] << II; + if (isLikelyTypo(Candidates, Matches[2], II->getName())) { + Diag << FixItHint::CreateReplacement( + Comment.first, (Matches[1] + II->getName() + Matches[3]).str()); + } + } + Self.diag(DeclLoc, "%0 declared here", DiagnosticIDs::Note) << II; + return true; +} + // Given the argument type and the options determine if we should // be adding an argument comment. bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const { @@ -342,25 +368,13 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, ArgBeginLoc = Args[I]->getEndLoc(); for (auto Comment : Comments) { - llvm::SmallVector<StringRef, 2> Matches; - if (IdentRE.match(Comment.second, &Matches) && - !sameName(Matches[2], II->getName(), StrictMode)) { - { - const DiagnosticBuilder Diag = - diag(Comment.first, "argument name '%0' in comment does not " - "match parameter name %1") - << Matches[2] << II; - if (isLikelyTypo(Callee->parameters(), Matches[2], II->getName())) { - Diag << FixItHint::CreateReplacement( - Comment.first, (Matches[1] + II->getName() + Matches[3]).str()); - } - } - diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note) << II; - if (OriginalCallee != Callee) { - diag(OriginalCallee->getLocation(), - "actual callee (%0) is declared here", DiagnosticIDs::Note) - << OriginalCallee; - } + const bool HadMismatch = diagnoseMismatchInComment( + *this, Ctx, Callee->parameters(), II, StrictMode, IdentRE, Comment, + PVD->getLocation()); + if (HadMismatch && OriginalCallee != Callee) { + diag(OriginalCallee->getLocation(), + "actual callee (%0) is declared here", DiagnosticIDs::Note) + << OriginalCallee; } } >From 6a967c09482d1b70ae38a751c260f3961c4aeec1 Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Wed, 10 Dec 2025 22:39:49 +0800 Subject: [PATCH 04/10] [clang-tidy] Optimize string handling in ArgumentCommentCheck --- .../clang-tidy/bugprone/ArgumentCommentCheck.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index 41b571f46dfdc..81765824fe25f 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -380,8 +380,8 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, // If the argument comments are missing for literals add them. if (Comments.empty() && shouldAddComment(Args[I])) { - const std::string ArgComment = - (llvm::Twine("/*") + II->getName() + "=*/").str(); + llvm::SmallString<32> ArgComment; + (llvm::Twine("/*") + II->getName() + "=*/").toStringRef(ArgComment); const DiagnosticBuilder Diag = diag(Args[I]->getBeginLoc(), "argument comment missing for literal argument %0") >From fbbcf7882e0b5dfb6a494a0205b9a4246e5ccbcb Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Wed, 10 Dec 2025 22:46:44 +0800 Subject: [PATCH 05/10] [clang-tidy] Add support for struct initialization in bugprone-argument-comment --- .../bugprone/ArgumentCommentCheck.cpp | 78 ++++++++++++++++++- .../bugprone/ArgumentCommentCheck.h | 2 + 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index 81765824fe25f..43ada08d8801c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -75,6 +75,7 @@ void ArgumentCommentCheck::registerMatchers(MatchFinder *Finder) { isFromStdNamespaceOrSystemHeader()))) .bind("expr"), this); + Finder->addMatcher(initListExpr().bind("expr"), this); } static std::vector<std::pair<SourceLocation, StringRef>> @@ -391,6 +392,71 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, } } +void ArgumentCommentCheck::checkInitList(ASTContext *Ctx, + const InitListExpr *InitList) { + const QualType Type = InitList->getType(); + if (!Type->isRecordType()) + return; + const RecordDecl *RD = Type->getAsRecordDecl(); + if (!RD || RD->isUnion() || + (isa<CXXRecordDecl>(RD) && !cast<CXXRecordDecl>(RD)->isAggregate())) + return; + + if (const auto *InitListSyntactic = InitList->getSyntacticForm()) + InitList = InitListSyntactic; + + if (InitList->getNumInits() == 0 || + (IgnoreSingleArgument && InitList->getNumInits() == 1)) + return; + + // If any designated initializers are present, we don't try to apply + // positional logic for argument comments, as the structure is no longer + // a simple positional match. + for (unsigned I = 0; I < InitList->getNumInits(); ++I) { + if (InitList->getInit(I) && isa<DesignatedInitExpr>(InitList->getInit(I))) + return; + } + + SourceLocation ArgBeginLoc = InitList->getLBraceLoc(); + unsigned InitIndex = 0; + for (const FieldDecl *FD : RD->fields()) { + if (InitIndex >= InitList->getNumInits()) + break; + + if (FD->isUnnamedBitField()) + continue; + + const Expr *Arg = InitList->getInit(InitIndex); + + const IdentifierInfo *II = FD->getIdentifier(); + if (!II) { + ArgBeginLoc = Arg->getEndLoc(); + InitIndex++; + continue; + } + + llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> Comments = + collectLeadingComments(Ctx, ArgBeginLoc, Arg); + ArgBeginLoc = Arg->getEndLoc(); + + for (auto Comment : Comments) + (void)diagnoseMismatchInComment(*this, Ctx, RD->fields(), II, StrictMode, + IdentRE, Comment, FD->getLocation()); + + if (Comments.empty() && shouldAddComment(Arg)) { + llvm::SmallString<32> ArgComment; + (llvm::Twine("/*") + II->getName() + "=*/").toStringRef(ArgComment); + const DiagnosticBuilder Diag = + diag(Arg->getBeginLoc(), + "argument comment missing for literal argument %0") + << II + << FixItHint::CreateInsertion(Arg->getBeginLoc(), ArgComment); + } + + InitIndex++; + } +} + void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) { const auto *E = Result.Nodes.getNodeAs<Expr>("expr"); if (const auto *Call = dyn_cast<CallExpr>(E)) { @@ -400,8 +466,10 @@ void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) { checkCallArgs(Result.Context, Callee, Call->getCallee()->getEndLoc(), llvm::ArrayRef(Call->getArgs(), Call->getNumArgs())); - } else { - const auto *Construct = cast<CXXConstructExpr>(E); + return; + } + + if (const auto *Construct = dyn_cast<CXXConstructExpr>(E)) { if (Construct->getNumArgs() > 0 && Construct->getArg(0)->getSourceRange() == Construct->getSourceRange()) { // Ignore implicit construction. @@ -411,6 +479,12 @@ void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) { Result.Context, Construct->getConstructor(), Construct->getParenOrBraceRange().getBegin(), llvm::ArrayRef(Construct->getArgs(), Construct->getNumArgs())); + return; + } + + if (const auto *InitList = dyn_cast<InitListExpr>(E)) { + checkInitList(Result.Context, InitList); + return; } } diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h index 30fa32fad72e7..ae8df41a30a22 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h @@ -53,6 +53,8 @@ class ArgumentCommentCheck : public ClangTidyCheck { SourceLocation ArgBeginLoc, llvm::ArrayRef<const Expr *> Args); + void checkInitList(ASTContext *Ctx, const InitListExpr *InitList); + bool shouldAddComment(const Expr *Arg) const; }; >From ed3d8ca1779b9a90282a48d3f8f49da752914a86 Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Wed, 10 Dec 2025 22:48:43 +0800 Subject: [PATCH 06/10] [clang-tidy] Add tests for argument-comment struct initialization --- .../bugprone/argument-comment-struct-init.cpp | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp new file mode 100644 index 0000000000000..8f8b5e0a17f57 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp @@ -0,0 +1,56 @@ +// RUN: %check_clang_tidy %s bugprone-argument-comment %t + +struct A { + int x, y; +}; + +void testA() { + A a1 = {/*x=*/1, /*y=*/2}; + A a2 = {/*y=*/1, /*x=*/2}; + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment] + // CHECK-MESSAGES: [[@LINE-2]]:20: warning: argument name 'x' in comment does not match parameter name 'y' [bugprone-argument-comment] + + A a3 = { + /*x=*/1, + /*y=*/2 + }; + + A a4 = { + /*y=*/1, + /*x=*/2 + }; + // CHECK-MESSAGES: [[@LINE-3]]:5: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment] + // CHECK-MESSAGES: [[@LINE-3]]:5: warning: argument name 'x' in comment does not match parameter name 'y' [bugprone-argument-comment] +} + +struct B { + int x, y, z; +}; + +void testB() { + B b1 = {/*x=*/1, /*y=*/2}; // Partial init + B b2 = {/*z=*/1, /*y=*/2}; // Typo x->z + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument name 'z' in comment does not match parameter name 'x' [bugprone-argument-comment] +} + +struct BitFields { + int a : 4; + int : 4; + int b : 4; +}; + +void testBitFields() { + BitFields b1 = {/*a=*/1, /*b=*/2}; + BitFields b2 = {/*a=*/1, /*c=*/2}; + // CHECK-MESSAGES: [[@LINE-1]]:28: warning: argument name 'c' in comment does not match parameter name 'b' [bugprone-argument-comment] +} + +struct CorrectFix { + int long_field_name; + int other; +}; + +void testFix() { + CorrectFix c = {/*long_feild_name=*/1, 2}; + // CHECK-MESSAGES: [[@LINE-1]]:19: warning: argument name 'long_feild_name' in comment does not match parameter name 'long_field_name' [bugprone-argument-comment] +} >From 01acc65ff67bc3e2f378e74242b1fbe2e6842b79 Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Wed, 10 Dec 2025 23:43:51 +0800 Subject: [PATCH 07/10] [clang-tidy] Add support for inheritance structs --- .../bugprone/ArgumentCommentCheck.cpp | 113 +++++++++++------- .../bugprone/ArgumentCommentCheck.h | 6 +- .../bugprone/argument-comment-struct-init.cpp | 18 ++- 3 files changed, 90 insertions(+), 47 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index 43ada08d8801c..5eef0653250e3 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -157,9 +157,9 @@ static bool isLikelyTypo(const NamedDeclRange &Candidates, StringRef ArgName, TargetNameLower.reserve(TargetName.size()); for (char C : TargetName) TargetNameLower.push_back(llvm::toLower(C)); - const unsigned ThisED = ArgNameLowerRef.edit_distance( - StringRef(TargetNameLower), - /*AllowReplacements=*/true, UpperBound); + const unsigned ThisED = + ArgNameLowerRef.edit_distance(StringRef(TargetNameLower), + /*AllowReplacements=*/true, UpperBound); if (ThisED >= UpperBound) return false; @@ -264,8 +264,7 @@ static const FunctionDecl *resolveMocks(const FunctionDecl *Func) { static CharSourceRange makeFileCharRange(ASTContext *Ctx, SourceLocation Begin, SourceLocation End) { return Lexer::makeFileCharRange(CharSourceRange::getCharRange(Begin, End), - Ctx->getSourceManager(), - Ctx->getLangOpts()); + Ctx->getSourceManager(), Ctx->getLangOpts()); } // Collects consecutive comments immediately preceding an argument expression. @@ -294,9 +293,8 @@ collectLeadingComments(ASTContext *Ctx, SourceLocation SearchBegin, template <typename NamedDeclRange> static bool diagnoseMismatchInComment( ArgumentCommentCheck &Self, ASTContext *Ctx, - const NamedDeclRange &Candidates, const IdentifierInfo *II, - bool StrictMode, llvm::Regex &IdentRE, - const std::pair<SourceLocation, StringRef> &Comment, + const NamedDeclRange &Candidates, const IdentifierInfo *II, bool StrictMode, + llvm::Regex &IdentRE, const std::pair<SourceLocation, StringRef> &Comment, SourceLocation DeclLoc) { llvm::SmallVector<StringRef, 2> Matches; if (!(IdentRE.match(Comment.second, &Matches) && @@ -304,10 +302,11 @@ static bool diagnoseMismatchInComment( return false; { - const DiagnosticBuilder Diag = Self.diag( - Comment.first, - "argument name '%0' in comment does not match parameter name %1") - << Matches[2] << II; + const DiagnosticBuilder Diag = + Self.diag( + Comment.first, + "argument name '%0' in comment does not match parameter name %1") + << Matches[2] << II; if (isLikelyTypo(Candidates, Matches[2], II->getName())) { Diag << FixItHint::CreateReplacement( Comment.first, (Matches[1] + II->getName() + Matches[3]).str()); @@ -392,33 +391,29 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, } } -void ArgumentCommentCheck::checkInitList(ASTContext *Ctx, - const InitListExpr *InitList) { - const QualType Type = InitList->getType(); - if (!Type->isRecordType()) - return; - const RecordDecl *RD = Type->getAsRecordDecl(); - if (!RD || RD->isUnion() || - (isa<CXXRecordDecl>(RD) && !cast<CXXRecordDecl>(RD)->isAggregate())) - return; - - if (const auto *InitListSyntactic = InitList->getSyntacticForm()) - InitList = InitListSyntactic; - - if (InitList->getNumInits() == 0 || - (IgnoreSingleArgument && InitList->getNumInits() == 1)) - return; - - // If any designated initializers are present, we don't try to apply - // positional logic for argument comments, as the structure is no longer - // a simple positional match. - for (unsigned I = 0; I < InitList->getNumInits(); ++I) { - if (InitList->getInit(I) && isa<DesignatedInitExpr>(InitList->getInit(I))) - return; +static void checkRecordInitializer(ArgumentCommentCheck &Self, ASTContext *Ctx, + const RecordDecl *RD, + const InitListExpr *InitList, + unsigned &InitIndex, + SourceLocation &ArgBeginLoc) { + if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) { + for (const auto &Base : CXXRD->bases()) { + if (InitIndex >= InitList->getNumInits()) + return; + const RecordDecl *BaseRD = Base.getType()->getAsRecordDecl(); + if (BaseRD) { + checkRecordInitializer(Self, Ctx, BaseRD, InitList, InitIndex, + ArgBeginLoc); + } else { + // If we can't resolve the base record (e.g. template), skip the + // initializer. + if (const Expr *Arg = InitList->getInit(InitIndex)) + ArgBeginLoc = Arg->getEndLoc(); + InitIndex++; + } + } } - SourceLocation ArgBeginLoc = InitList->getLBraceLoc(); - unsigned InitIndex = 0; for (const FieldDecl *FD : RD->fields()) { if (InitIndex >= InitList->getNumInits()) break; @@ -440,23 +435,53 @@ void ArgumentCommentCheck::checkInitList(ASTContext *Ctx, ArgBeginLoc = Arg->getEndLoc(); for (auto Comment : Comments) - (void)diagnoseMismatchInComment(*this, Ctx, RD->fields(), II, StrictMode, - IdentRE, Comment, FD->getLocation()); + (void)diagnoseMismatchInComment(Self, Ctx, RD->fields(), II, + Self.isStrictMode(), Self.getIdentRE(), + Comment, FD->getLocation()); - if (Comments.empty() && shouldAddComment(Arg)) { + if (Comments.empty() && Self.shouldAddComment(Arg)) { llvm::SmallString<32> ArgComment; (llvm::Twine("/*") + II->getName() + "=*/").toStringRef(ArgComment); const DiagnosticBuilder Diag = - diag(Arg->getBeginLoc(), - "argument comment missing for literal argument %0") - << II - << FixItHint::CreateInsertion(Arg->getBeginLoc(), ArgComment); + Self.diag(Arg->getBeginLoc(), + "argument comment missing for literal argument %0") + << II << FixItHint::CreateInsertion(Arg->getBeginLoc(), ArgComment); } InitIndex++; } } +void ArgumentCommentCheck::checkInitList(ASTContext *Ctx, + const InitListExpr *InitList) { + const QualType Type = InitList->getType(); + if (!Type->isRecordType()) + return; + const RecordDecl *RD = Type->getAsRecordDecl(); + if (!RD || RD->isUnion() || + (isa<CXXRecordDecl>(RD) && !cast<CXXRecordDecl>(RD)->isAggregate())) + return; + + if (const auto *InitListSyntactic = InitList->getSyntacticForm()) + InitList = InitListSyntactic; + + if (InitList->getNumInits() == 0 || + (IgnoreSingleArgument && InitList->getNumInits() == 1)) + return; + + // If any designated initializers are present, we don't try to apply + // positional logic for argument comments, as the structure is no longer + // a simple positional match. + for (unsigned I = 0; I < InitList->getNumInits(); ++I) { + if (InitList->getInit(I) && isa<DesignatedInitExpr>(InitList->getInit(I))) + return; + } + + SourceLocation ArgBeginLoc = InitList->getLBraceLoc(); + unsigned InitIndex = 0; + checkRecordInitializer(*this, Ctx, RD, InitList, InitIndex, ArgBeginLoc); +} + void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) { const auto *E = Result.Nodes.getNodeAs<Expr>("expr"); if (const auto *Call = dyn_cast<CallExpr>(E)) { diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h index ae8df41a30a22..b48a677d96f6b 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h @@ -37,6 +37,10 @@ class ArgumentCommentCheck : public ClangTidyCheck { void check(const ast_matchers::MatchFinder::MatchResult &Result) override; void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + bool shouldAddComment(const Expr *Arg) const; + bool isStrictMode() const { return StrictMode; } + llvm::Regex &getIdentRE() { return IdentRE; } + private: const unsigned StrictMode : 1; const unsigned IgnoreSingleArgument : 1; @@ -54,8 +58,6 @@ class ArgumentCommentCheck : public ClangTidyCheck { llvm::ArrayRef<const Expr *> Args); void checkInitList(ASTContext *Ctx, const InitListExpr *InitList); - - bool shouldAddComment(const Expr *Arg) const; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp index 8f8b5e0a17f57..93d6ac1425601 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp @@ -1,4 +1,4 @@ -// RUN: %check_clang_tidy %s bugprone-argument-comment %t +// RUN: %check_clang_tidy -std=c++17 %s bugprone-argument-comment %t struct A { int x, y; @@ -54,3 +54,19 @@ void testFix() { CorrectFix c = {/*long_feild_name=*/1, 2}; // CHECK-MESSAGES: [[@LINE-1]]:19: warning: argument name 'long_feild_name' in comment does not match parameter name 'long_field_name' [bugprone-argument-comment] } + +struct Base { + int b; +}; + +struct Derived : Base { + int d; +}; + +void testInheritance() { + Derived d1 = {/*b=*/ 1, /*d=*/ 2}; + Derived d2 = {/*x=*/ 1, /*d=*/ 2}; + // CHECK-MESSAGES: [[@LINE-1]]:17: warning: argument name 'x' in comment does not match parameter name 'b' [bugprone-argument-comment] + Derived d3 = {/*b=*/ 1, /*x=*/ 2}; + // CHECK-MESSAGES: [[@LINE-1]]:27: warning: argument name 'x' in comment does not match parameter name 'd' [bugprone-argument-comment] +} >From 79aa9aa75c9613f7464a199ffefbaf3f34899d8c Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Thu, 11 Dec 2025 00:09:31 +0800 Subject: [PATCH 08/10] [clang-tidy] Fix bugs in inheritance implementation --- .../bugprone/ArgumentCommentCheck.cpp | 64 ++++++++++++++++--- .../bugprone/argument-comment-struct-init.cpp | 62 +++++++++++++++++- 2 files changed, 117 insertions(+), 9 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index 5eef0653250e3..fd789221fbe43 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -396,20 +396,68 @@ static void checkRecordInitializer(ArgumentCommentCheck &Self, ASTContext *Ctx, const InitListExpr *InitList, unsigned &InitIndex, SourceLocation &ArgBeginLoc) { + // If the record is not an aggregate (e.g. a base class with a user-declared + // constructor), we treat it as a single opaque element initialization. + bool IsAggregate = false; + if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) + IsAggregate = CXXRD->isAggregate(); + else + IsAggregate = true; // C struct + + if (!IsAggregate) { + if (InitIndex < InitList->getNumInits()) { + const Expr *Arg = InitList->getInit(InitIndex); + ArgBeginLoc = Arg->getEndLoc(); + InitIndex++; + } + return; + } + if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) { for (const auto &Base : CXXRD->bases()) { if (InitIndex >= InitList->getNumInits()) return; + const RecordDecl *BaseRD = Base.getType()->getAsRecordDecl(); - if (BaseRD) { - checkRecordInitializer(Self, Ctx, BaseRD, InitList, InitIndex, - ArgBeginLoc); - } else { - // If we can't resolve the base record (e.g. template), skip the - // initializer. - if (const Expr *Arg = InitList->getInit(InitIndex)) + if (!BaseRD) { + if (InitIndex < InitList->getNumInits()) { + const Expr *Arg = InitList->getInit(InitIndex); ArgBeginLoc = Arg->getEndLoc(); + InitIndex++; + } + continue; + } + + const Expr *NextInit = InitList->getInit(InitIndex); + QualType InitType = NextInit->getType(); + QualType BaseType = Base.getType(); + + // Check if this is an explicit initializer for the base. + const Expr *Stripped = NextInit->IgnoreParenImpCasts(); + const auto *SubInitList = dyn_cast<InitListExpr>(Stripped); + bool IsExplicitSubInit = + SubInitList && + Ctx->hasSameUnqualifiedType(SubInitList->getType(), BaseType); + + if (IsExplicitSubInit) { + unsigned SubIndex = 0; + SourceLocation SubLoc = SubInitList->getLBraceLoc(); + checkRecordInitializer(Self, Ctx, BaseRD, SubInitList, SubIndex, + SubLoc); + ArgBeginLoc = NextInit->getEndLoc(); InitIndex++; + } else if (Ctx->hasSameUnqualifiedType(InitType, BaseType) || + (InitType->isRecordType() && BaseType->isRecordType() && + cast<CXXRecordDecl>(InitType->getAsRecordDecl()) + ->isDerivedFrom(cast<CXXRecordDecl>(BaseRD)))) { + // Copy/move construction or conversion from derived class. + // Treat as a single scalar initializer for the base. + ArgBeginLoc = NextInit->getEndLoc(); + InitIndex++; + } else { + // Recursing into the current list. + checkRecordInitializer(Self, Ctx, BaseRD, InitList, InitIndex, + ArgBeginLoc); } } } @@ -473,7 +521,7 @@ void ArgumentCommentCheck::checkInitList(ASTContext *Ctx, // positional logic for argument comments, as the structure is no longer // a simple positional match. for (unsigned I = 0; I < InitList->getNumInits(); ++I) { - if (InitList->getInit(I) && isa<DesignatedInitExpr>(InitList->getInit(I))) + if (dyn_cast_or_null<DesignatedInitExpr>(InitList->getInit(I))) return; } diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp index 93d6ac1425601..0ee6ad7e8196d 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp @@ -29,7 +29,7 @@ struct B { void testB() { B b1 = {/*x=*/1, /*y=*/2}; // Partial init - B b2 = {/*z=*/1, /*y=*/2}; // Typo x->z + B b2 = {/*z=*/1, /*y=*/2}; // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument name 'z' in comment does not match parameter name 'x' [bugprone-argument-comment] } @@ -53,6 +53,7 @@ struct CorrectFix { void testFix() { CorrectFix c = {/*long_feild_name=*/1, 2}; // CHECK-MESSAGES: [[@LINE-1]]:19: warning: argument name 'long_feild_name' in comment does not match parameter name 'long_field_name' [bugprone-argument-comment] + // CHECK-FIXES: CorrectFix c = {/*long_field_name=*/1, 2}; } struct Base { @@ -70,3 +71,62 @@ void testInheritance() { Derived d3 = {/*b=*/ 1, /*x=*/ 2}; // CHECK-MESSAGES: [[@LINE-1]]:27: warning: argument name 'x' in comment does not match parameter name 'd' [bugprone-argument-comment] } + + +struct DerivedExplicit : Base { + int d; +}; + +void testInheritanceExplicit() { + DerivedExplicit d1 = {{/*b=*/ 1}, /*d=*/ 2}; + DerivedExplicit d2 = {{/*x=*/ 1}, /*d=*/ 2}; + // CHECK-MESSAGES: [[@LINE-1]]:26: warning: argument name 'x' in comment does not match parameter name 'b' [bugprone-argument-comment] +} + +struct DeepBase { + int db; +}; + +struct Middle : DeepBase { + int m; +}; + +struct DerivedDeep : Middle { + int d; +}; + +void testDeepInheritance() { + DerivedDeep d1 = {/*db=*/ 1, /*m=*/ 2, /*d=*/ 3}; + DerivedDeep d2 = {/*x=*/ 1, /*m=*/ 2, /*d=*/ 3}; + // CHECK-MESSAGES: [[@LINE-1]]:21: warning: argument name 'x' in comment does not match parameter name 'db' [bugprone-argument-comment] +} + +struct Inner { + int i; +}; + +struct Outer { + Inner in; +}; + +void testNestedStruct() { + Outer o = {/*i=*/1}; + // CHECK-MESSAGES: [[@LINE-1]]:14: warning: argument name 'i' in comment does not match parameter name 'in' [bugprone-argument-comment] +} + +#define MACRO_VAL 1 +void testMacroVal() { + A a = {/*x=*/ MACRO_VAL, /*y=*/ 2}; + A a2 = {/*y=*/ MACRO_VAL, /*x=*/ 2}; + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment] + // CHECK-MESSAGES: [[@LINE-2]]:29: warning: argument name 'x' in comment does not match parameter name 'y' [bugprone-argument-comment] +} + +#define MACRO_INIT { /*x=*/ 1, /*y=*/ 2 } +#define MACRO_INIT_BAD { /*y=*/ 1, /*x=*/ 2 } + +void testMacroInit() { + A a = MACRO_INIT; + A a2 = MACRO_INIT_BAD; + // Won't flag warnings. +} >From fbc5f7ff93a45549462070dc869d855dee750d0d Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Thu, 11 Dec 2025 00:16:03 +0800 Subject: [PATCH 09/10] [clang-tidy] Change the scope of checkRecordInitializer --- .../bugprone/ArgumentCommentCheck.cpp | 29 +++++++++---------- .../bugprone/ArgumentCommentCheck.h | 9 +++--- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index fd789221fbe43..6a20d4e5c17de 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -391,11 +391,11 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx, } } -static void checkRecordInitializer(ArgumentCommentCheck &Self, ASTContext *Ctx, - const RecordDecl *RD, - const InitListExpr *InitList, - unsigned &InitIndex, - SourceLocation &ArgBeginLoc) { +void ArgumentCommentCheck::checkRecordInitializer(ASTContext *Ctx, + const RecordDecl *RD, + const InitListExpr *InitList, + unsigned &InitIndex, + SourceLocation &ArgBeginLoc) { // If the record is not an aggregate (e.g. a base class with a user-declared // constructor), we treat it as a single opaque element initialization. bool IsAggregate = false; @@ -442,8 +442,7 @@ static void checkRecordInitializer(ArgumentCommentCheck &Self, ASTContext *Ctx, if (IsExplicitSubInit) { unsigned SubIndex = 0; SourceLocation SubLoc = SubInitList->getLBraceLoc(); - checkRecordInitializer(Self, Ctx, BaseRD, SubInitList, SubIndex, - SubLoc); + checkRecordInitializer(Ctx, BaseRD, SubInitList, SubIndex, SubLoc); ArgBeginLoc = NextInit->getEndLoc(); InitIndex++; } else if (Ctx->hasSameUnqualifiedType(InitType, BaseType) || @@ -456,8 +455,7 @@ static void checkRecordInitializer(ArgumentCommentCheck &Self, ASTContext *Ctx, InitIndex++; } else { // Recursing into the current list. - checkRecordInitializer(Self, Ctx, BaseRD, InitList, InitIndex, - ArgBeginLoc); + checkRecordInitializer(Ctx, BaseRD, InitList, InitIndex, ArgBeginLoc); } } } @@ -483,16 +481,15 @@ static void checkRecordInitializer(ArgumentCommentCheck &Self, ASTContext *Ctx, ArgBeginLoc = Arg->getEndLoc(); for (auto Comment : Comments) - (void)diagnoseMismatchInComment(Self, Ctx, RD->fields(), II, - Self.isStrictMode(), Self.getIdentRE(), - Comment, FD->getLocation()); + (void)diagnoseMismatchInComment(*this, Ctx, RD->fields(), II, StrictMode, + IdentRE, Comment, FD->getLocation()); - if (Comments.empty() && Self.shouldAddComment(Arg)) { + if (Comments.empty() && shouldAddComment(Arg)) { llvm::SmallString<32> ArgComment; (llvm::Twine("/*") + II->getName() + "=*/").toStringRef(ArgComment); const DiagnosticBuilder Diag = - Self.diag(Arg->getBeginLoc(), - "argument comment missing for literal argument %0") + diag(Arg->getBeginLoc(), + "argument comment missing for literal argument %0") << II << FixItHint::CreateInsertion(Arg->getBeginLoc(), ArgComment); } @@ -527,7 +524,7 @@ void ArgumentCommentCheck::checkInitList(ASTContext *Ctx, SourceLocation ArgBeginLoc = InitList->getLBraceLoc(); unsigned InitIndex = 0; - checkRecordInitializer(*this, Ctx, RD, InitList, InitIndex, ArgBeginLoc); + checkRecordInitializer(Ctx, RD, InitList, InitIndex, ArgBeginLoc); } void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) { diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h index b48a677d96f6b..9e9df06b0d54b 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h @@ -37,10 +37,6 @@ class ArgumentCommentCheck : public ClangTidyCheck { void check(const ast_matchers::MatchFinder::MatchResult &Result) override; void storeOptions(ClangTidyOptions::OptionMap &Opts) override; - bool shouldAddComment(const Expr *Arg) const; - bool isStrictMode() const { return StrictMode; } - llvm::Regex &getIdentRE() { return IdentRE; } - private: const unsigned StrictMode : 1; const unsigned IgnoreSingleArgument : 1; @@ -58,6 +54,11 @@ class ArgumentCommentCheck : public ClangTidyCheck { llvm::ArrayRef<const Expr *> Args); void checkInitList(ASTContext *Ctx, const InitListExpr *InitList); + void checkRecordInitializer(ASTContext *Ctx, const RecordDecl *RD, + const InitListExpr *InitList, unsigned &InitIndex, + SourceLocation &ArgBeginLoc); + + bool shouldAddComment(const Expr *Arg) const; }; } // namespace clang::tidy::bugprone >From bf0b0079093bd27642aaeee5f3c24e58b49d003a Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Thu, 11 Dec 2025 00:23:00 +0800 Subject: [PATCH 10/10] [clang-tidy] Add docs --- clang-tools-extra/docs/ReleaseNotes.rst | 5 +++++ .../checks/bugprone/argument-comment.rst | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index d1fb1cba3e45a..3616c404fcfc0 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -331,6 +331,11 @@ New check aliases Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:`bugprone-argument-comment + <clang-tidy/checks/bugprone/argument-comment>` check to support C++ + aggregate initialization, including validation of comments against + struct field names and inheritance support. + - Improved :doc:`bugprone-easily-swappable-parameters <clang-tidy/checks/bugprone/easily-swappable-parameters>` check by correcting a spelling mistake on its option diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst index 8770d7224137a..29a1756be1c1e 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst @@ -19,6 +19,21 @@ that are placed right before the argument. The check tries to detect typos and suggest automated fixes for them. +The check also supports C++ aggregate initialization, allowing comments to be +validated against struct and class field names, including those inherited from +base classes. + +.. code-block:: c++ + + struct Base { int b; }; + struct Derived : Base { int d; }; + + void f() { + Derived d1 = {/*b=*/1, /*d=*/2}; // OK + Derived d2 = {/*x=*/1, /*d=*/2}; + // warning: argument name 'x' in comment does not match parameter name 'b' + } + Options ------- _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
