[clang-tools-extra] [clang-tidy] Add support for struct initialization in `bugprone-argument-comment` (PR #171757)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 11 07:47:42 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: mitchell (zeyi2)
<details>
<summary>Changes</summary>
Closes #<!-- -->170921
---
Patch is 21.97 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/171757.diff
5 Files Affected:
- (modified) clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp (+245-57)
- (modified) clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h (+5)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
- (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst (+15)
- (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp (+132)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
index ed30d01e645d1..7ae84003655bf 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>>
@@ -142,31 +143,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 (const 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(),
- /*AllowReplacements=*/true, UpperBound);
+ llvm::SmallString<64> TargetNameLower;
+ TargetNameLower.reserve(TargetName.size());
+ for (const 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;
+ const StringRef CandName = II->getName();
+ CandidateLower.reserve(CandName.size());
+ for (const 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;
@@ -243,6 +259,63 @@ 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;
+}
+
+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 {
@@ -274,12 +347,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();
@@ -296,47 +363,25 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
}
}
- const CharSourceRange BeforeArgument =
- MakeFileCharRange(ArgBeginLoc, Args[I]->getBeginLoc());
+ const 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) &&
- !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], I)) {
- 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;
}
}
// 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")
@@ -346,6 +391,142 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
}
}
+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;
+ 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) {
+ if (InitIndex < InitList->getNumInits()) {
+ const Expr *Arg = InitList->getInit(InitIndex);
+ ArgBeginLoc = Arg->getEndLoc();
+ InitIndex++;
+ }
+ continue;
+ }
+
+ const Expr *NextInit = InitList->getInit(InitIndex);
+ const QualType InitType = NextInit->getType();
+ const 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);
+ const bool IsExplicitSubInit =
+ SubInitList &&
+ ASTContext::hasSameUnqualifiedType(SubInitList->getType(), BaseType);
+
+ if (IsExplicitSubInit) {
+ unsigned SubIndex = 0;
+ SourceLocation SubLoc = SubInitList->getLBraceLoc();
+ checkRecordInitializer(Ctx, BaseRD, SubInitList, SubIndex, SubLoc);
+ ArgBeginLoc = NextInit->getEndLoc();
+ InitIndex++;
+ } else if (ASTContext::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(Ctx, BaseRD, InitList, InitIndex, ArgBeginLoc);
+ }
+ }
+ }
+
+ 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;
+ }
+
+ const 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::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 (dyn_cast_or_null<DesignatedInitExpr>(InitList->getInit(I)))
+ return;
+ }
+
+ SourceLocation ArgBeginLoc = InitList->getLBraceLoc();
+ unsigned InitIndex = 0;
+ checkRecordInitializer(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)) {
@@ -355,8 +536,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.
@@ -366,6 +549,11 @@ 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);
}
}
diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
index 30fa32fad72e7..9e9df06b0d54b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
@@ -53,6 +53,11 @@ class ArgumentCommentCheck : public ClangTidyCheck {
SourceLocation ArgBeginLoc,
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;
};
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
-------
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..0ee6ad7e8196d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp
@@ -0,0 +1,132 @@
+// RUN: %check_clang_tidy -std=c++17 %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};
+ // 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]
+ // CHECK-FIXES: CorrectFix c = {/*long_field_name=*/1, 2};
+}
+
+struct Base {
+ int b;
+};
+
+struct Derived : Base {
+ in...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/171757
More information about the cfe-commits
mailing list