Skip to content

[clang-tidy][NFC] Refactor bugprone-argument-comment [1/3] #172521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zeyi2 wants to merge 5 commits into
base: main
Choose a base branch
from
+36 −25
Open

[clang-tidy][NFC] Refactor bugprone-argument-comment [1/3] #172521

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1c7bfc
[clang-tidy] Refactor isLikelyTypo to be a template function
zeyi2 Dec 10, 2025
96323fd
[clang-tidy] Optimize string handling in ArgumentCommentCheck
zeyi2 Dec 16, 2025
e64e515
Fix formatting
zeyi2 Dec 16, 2025
7b101b2
Fix clang-tidy warnings, though it may be ugly..
zeyi2 Dec 16, 2025
ae5b6e8
use `llvm::all_of`
zeyi2 Dec 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 36 additions & 25 deletions clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,37 +142,48 @@ 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;
static llvm::SmallString<64> getLowercasedString(StringRef Name) {
Copy link
Member Author

@zeyi2 zeyi2 Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added mainly to avoid the use of std::string

llvm::SmallString<64> Result;
Result.reserve(Name.size());
for (const char C : Name)
Result.push_back(llvm::toLower(C));
return Result;
}
Comment on lines +145 to +151
Copy link
Contributor

@vbvictor vbvictor Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for pushback, but
TBH, I'm not a fan of avoiding std::string at all cost because it brings more temporary variables (like CandidateLower, TargetNameLower) and new functions getLowercasedString which make code bigger thus harder to read. And there may be no performance benefit to it.

If we are to avoid allocations, we should probably change StringRef::lower to do only 1 allocation instead of current

std::string StringRef::lower() const {
  return std::string(map_iterator(begin(), toLower),
                     map_iterator(end(), toLower));
}

We can do

std::string StringRef::lower() const {
  std::string res;
  res.reserve(this->size());
  // fill via cycle or smth
}


template <typename NamedDeclRange>
static bool isLikelyTypo(const NamedDeclRange &Candidates, StringRef ArgName,
StringRef TargetName) {
const llvm::SmallString<64> ArgNameLower = getLowercasedString(ArgName);
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);
const llvm::SmallString<64> TargetNameLower = getLowercasedString(TargetName);
const unsigned ThisED =
ArgNameLowerRef.edit_distance(StringRef(TargetNameLower),
/*AllowReplacements=*/true, UpperBound);
Comment on lines +162 to +163
Copy link
Contributor

@zwuis zwuis Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

-edit_distance(StringRef(TargetNameLower), /*AllowReplacements=*/true, UpperBound);
+edit_distance(TargetNameLower, /*AllowReplacements=*/true, UpperBound);

It seems that implicit conversions here will not cause any bugs.

Ditto elsewhere.

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();
return llvm::all_of(Candidates, [&](const auto &Candidate) {
const IdentifierInfo *II = Candidate->getIdentifier();
if (II->getName() == TargetName)
return true;

if (!II)
continue;
return true;

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.
const llvm::SmallString<64> CandidateLower =
getLowercasedString(II->getName());
const unsigned OtherED = ArgNameLowerRef.edit_distance(
StringRef(CandidateLower),
/*AllowReplacements=*/true, ThisED + Threshold);
if (OtherED < ThisED + Threshold)
return false;
}

return true;
return OtherED >= ThisED + Threshold;
});
}

static bool sameName(StringRef InComment, StringRef InDecl, bool StrictMode) {
Expand Down Expand Up @@ -319,7 +330,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());
}
Expand All @@ -335,8 +346,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")
Expand Down