https://github.com/daandemeyer created https://github.com/llvm/llvm-project/pull/237890
Many C APIs take 0 to mean "no flags" or "the default", and a bare 0 doesn't tell the reader which parameter it sets. CommentIntegerLiterals of bugprone-argument-comment asks for a comment on every integer literal though, including sizes, modes and counts whose meaning is clear from their value. Let's turn CommentIntegerLiterals into an enum with the values None, ZeroOnly and All. ZeroOnly only asks for a comment on integer literals with the value zero. The option keeps accepting true and false, which mean All and None, so existing configurations keep working. --dump-config prints All or None for them. Split out of https://github.com/llvm/llvm-project/pull/237623. >From 0fa0f6cf575f2194471c208a08b89d01432492f7 Mon Sep 17 00:00:00 2001 From: Daan De Meyer <[email protected]> Date: Mon, 2 Nov 2026 10:34:55 +0100 Subject: [PATCH] [clang-tidy] Add ZeroOnly integer literal mode to argument-comment Many C APIs take 0 to mean "no flags" or "the default", and a bare 0 doesn't tell the reader which parameter it sets. CommentIntegerLiterals of bugprone-argument-comment asks for a comment on every integer literal though, including sizes, modes and counts whose meaning is clear from their value. Let's turn CommentIntegerLiterals into an enum with the values None, ZeroOnly and All. ZeroOnly only asks for a comment on integer literals with the value zero. We check the value before stripping a unary operator, because ~0 and !0 aren't zero. The option keeps accepting true and false, which mean All and None, so existing configurations keep working. Signed-off-by: Daan De Meyer <[email protected]> --- .../bugprone/ArgumentCommentCheck.cpp | 41 ++++++++++++++++- .../bugprone/ArgumentCommentCheck.h | 13 +++++- clang-tools-extra/docs/ReleaseNotes.md | 6 +++ .../checks/bugprone/argument-comment.md | 29 ++++++++++-- .../argument-comment-integer-literals.cpp | 46 +++++++++++++++++++ 5 files changed, 128 insertions(+), 7 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-integer-literals.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp index 783944cd1a5a17..e3506f42b39e41 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp @@ -11,11 +11,25 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" #include "clang/Lex/Token.h" +#include "llvm/Support/YAMLParser.h" #include "../utils/LexerUtils.h" using namespace clang::ast_matchers; +namespace clang::tidy { +using IntegerLiteralKind = bugprone::ArgumentCommentCheck::IntegerLiteralKind; + +llvm::ArrayRef<std::pair<IntegerLiteralKind, StringRef>> +OptionEnumMapping<IntegerLiteralKind>::getEnumMapping() { + static constexpr std::pair<IntegerLiteralKind, StringRef> Mapping[] = { + {IntegerLiteralKind::None, "None"}, + {IntegerLiteralKind::ZeroOnly, "ZeroOnly"}, + {IntegerLiteralKind::All, "All"}}; + return {Mapping}; +} +} // namespace clang::tidy + namespace clang::tidy::bugprone { using utils::lexer::CommentToken; @@ -31,6 +45,22 @@ AST_MATCHER(Decl, isFromStdNamespaceOrSystemHeader) { } } // namespace +// CommentIntegerLiterals used to be a boolean option, so existing `.clang-tidy` +// files set it to `true` or `false`. Accept the spellings that +// OptionsView::get<bool>() accepts, as All and None. +static IntegerLiteralKind +getCommentIntegerLiterals(const ClangTidyCheck::OptionsView &Options) { + if (const std::optional<StringRef> Value = + Options.get("CommentIntegerLiterals")) { + if (const std::optional<bool> Bool = llvm::yaml::parseBool(*Value)) + return *Bool ? IntegerLiteralKind::All : IntegerLiteralKind::None; + long long Number = 0; + if (!Value->getAsInteger(10, Number)) + return Number != 0 ? IntegerLiteralKind::All : IntegerLiteralKind::None; + } + return Options.get("CommentIntegerLiterals", IntegerLiteralKind::None); +} + ArgumentCommentCheck::ArgumentCommentCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), @@ -41,7 +71,6 @@ ArgumentCommentCheck::ArgumentCommentCheck(StringRef Name, CommentBoolLiterals(Options.get("CommentBoolLiterals", false)), CommentCharacterLiterals(Options.get("CommentCharacterLiterals", false)), CommentFloatLiterals(Options.get("CommentFloatLiterals", false)), - CommentIntegerLiterals(Options.get("CommentIntegerLiterals", false)), CommentNullPtrs(Options.get("CommentNullPtrs", false)), CommentParenthesizedTemporaries( Options.get("CommentParenthesizedTemporaries", false)), @@ -49,6 +78,7 @@ ArgumentCommentCheck::ArgumentCommentCheck(StringRef Name, CommentTypedInitLists(Options.get("CommentTypedInitLists", false)), CommentUserDefinedLiterals( Options.get("CommentUserDefinedLiterals", false)), + CommentIntegerLiterals(getCommentIntegerLiterals(Options)), IdentRE("^(/\\* *)([_A-Za-z][_A-Za-z0-9]*)( *= *\\*/)$") {} void ArgumentCommentCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { @@ -272,6 +302,10 @@ ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const { // Strip implicit wrappers so brace-init arguments bound to references still // look like list-initialization at this point. Arg = Arg->IgnoreImplicit(); + // Check the value before stripping the unary operator, because `~0` and `!0` + // are not zero. + const auto *IntegerLit = dyn_cast<IntegerLiteral>(Arg); + const bool IsZero = IntegerLit && IntegerLit->getValue().isZero(); if (const auto *UO = dyn_cast<UnaryOperator>(Arg)) Arg = UO->getSubExpr()->IgnoreImplicit(); if (Arg->getExprLoc().isMacroID()) @@ -283,8 +317,11 @@ ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const { return CommentKind::NonLiteral; } + const bool CommentInteger = + CommentIntegerLiterals == IntegerLiteralKind::All || + (CommentIntegerLiterals == IntegerLiteralKind::ZeroOnly && IsZero); if ((CommentBoolLiterals && isa<CXXBoolLiteralExpr>(Arg)) || - (CommentIntegerLiterals && isa<IntegerLiteral>(Arg)) || + (CommentInteger && isa<IntegerLiteral>(Arg)) || (CommentFloatLiterals && isa<FloatingLiteral>(Arg)) || (CommentUserDefinedLiterals && isa<UserDefinedLiteral>(Arg)) || (CommentCharacterLiterals && isa<CharacterLiteral>(Arg)) || diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h index 3bed1ad4247e5b..c3810c51bde7bd 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h @@ -39,6 +39,8 @@ class ArgumentCommentCheck : public ClangTidyCheck { void check(const ast_matchers::MatchFinder::MatchResult &Result) override; void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + enum class IntegerLiteralKind { None, ZeroOnly, All }; + private: enum class CommentKind { None, @@ -52,12 +54,12 @@ class ArgumentCommentCheck : public ClangTidyCheck { const unsigned CommentBoolLiterals : 1; const unsigned CommentCharacterLiterals : 1; const unsigned CommentFloatLiterals : 1; - const unsigned CommentIntegerLiterals : 1; const unsigned CommentNullPtrs : 1; const unsigned CommentParenthesizedTemporaries : 1; const unsigned CommentStringLiterals : 1; const unsigned CommentTypedInitLists : 1; const unsigned CommentUserDefinedLiterals : 1; + const IntegerLiteralKind CommentIntegerLiterals; llvm::Regex IdentRE; void checkCallArgs(ASTContext *Ctx, const FunctionDecl *Callee, @@ -69,4 +71,13 @@ class ArgumentCommentCheck : public ClangTidyCheck { } // namespace clang::tidy::bugprone +namespace clang::tidy { +template <> +struct OptionEnumMapping<bugprone::ArgumentCommentCheck::IntegerLiteralKind> { + static ArrayRef< + std::pair<bugprone::ArgumentCommentCheck::IntegerLiteralKind, StringRef>> + getEnumMapping(); +}; +} // namespace clang::tidy + #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ARGUMENTCOMMENTCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 679ca9ecbb4a39..481fb8c2893a49 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -184,6 +184,12 @@ infrastructure are described first, followed by tool-specific sections. #### Changes in existing checks +- Improved {doc}`bugprone-argument-comment + <clang-tidy/checks/bugprone/argument-comment>` check by adding the + `ZeroOnly` value to the {option}`CommentIntegerLiterals` option, which only + adds comments to integer literals with the value zero. The option still + accepts `true` and `false`. + - Improved {doc}`bugprone-assignment-in-if-condition <clang-tidy/checks/bugprone/assignment-in-if-condition>` check by ignoring assignments inside GNU statement expressions unless they form the value of diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.md b/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.md index 68f7a6eb7a260f..c7ab5dbcbfcf56 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.md +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.md @@ -122,9 +122,12 @@ foo(/*Pi=*/3.14159); ``` ```{option} CommentIntegerLiterals -When `true`, the check will add argument comments in the format -`/*ParameterName=*/` right before the integer literal argument. -Default is `false`. +When `All`, the check will add argument comments in the format +`/*ParameterName=*/` right before every integer literal argument. When +`ZeroOnly`, it adds them before integer literal arguments with the value zero, +such as `0`, `0x0` and `0UL`, which often stand for "none" or "default". When +`None`, it adds no comments before integer literal arguments. The values `true` +and `false` mean `All` and `None`. Default is `None`. ``` Before: @@ -135,7 +138,7 @@ void foo(int MeaningOfLife); foo(42); ``` -After: +After, with `All`: ```c++ void foo(int MeaningOfLife); @@ -143,6 +146,24 @@ void foo(int MeaningOfLife); foo(/*MeaningOfLife=*/42); ``` +Before: + +```c +int open_file(const char *Path, int Flags); + +open_file(Path, 0); +open_file(Path, 1); +``` + +After, with `ZeroOnly`: + +```c +int open_file(const char *Path, int Flags); + +open_file(Path, /*Flags=*/0); +open_file(Path, 1); +``` + ```{option} CommentNullPtrs When `true`, the check will add argument comments in the format `/*ParameterName=*/` right before the nullptr literal argument. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-integer-literals.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-integer-literals.cpp new file mode 100644 index 00000000000000..15c2a46c31ab16 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-integer-literals.cpp @@ -0,0 +1,46 @@ +// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- \ +// RUN: -config="{CheckOptions: { \ +// RUN: bugprone-argument-comment.CommentIntegerLiterals: ZeroOnly}}" -- +// RUN: %check_clang_tidy -check-suffix=ALL %s bugprone-argument-comment %t -- \ +// RUN: -config="{CheckOptions: { \ +// RUN: bugprone-argument-comment.CommentIntegerLiterals: All}}" -- +// RUN: %check_clang_tidy -check-suffix=INVALID %s bugprone-argument-comment %t -- \ +// RUN: -config="{CheckOptions: { \ +// RUN: bugprone-argument-comment.CommentIntegerLiterals: Sometimes}}" -- + +// CHECK-MESSAGES-INVALID: warning: invalid configuration value 'Sometimes' for option 'bugprone-argument-comment.CommentIntegerLiterals' [clang-tidy-config] + +void f(int flags, unsigned long size, double ratio, bool force); + +#define ZERO 0 + +void test() { + f(0, 0UL, 0.0, false); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for literal argument 'flags' [bugprone-argument-comment] + // CHECK-MESSAGES: [[@LINE-2]]:8: warning: argument comment missing for literal argument 'size' [bugprone-argument-comment] + // CHECK-MESSAGES-ALL: [[@LINE-3]]:5: warning: argument comment missing for literal argument 'flags' [bugprone-argument-comment] + // CHECK-MESSAGES-ALL: [[@LINE-4]]:8: warning: argument comment missing for literal argument 'size' [bugprone-argument-comment] + // CHECK-FIXES: f(/*flags=*/0, /*size=*/0UL, 0.0, false); + // CHECK-FIXES-ALL: f(/*flags=*/0, /*size=*/0UL, 0.0, false); + + f(0x0, 4096, 1.0, true); + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: argument comment missing for literal argument 'flags' [bugprone-argument-comment] + // CHECK-MESSAGES-ALL: [[@LINE-2]]:5: warning: argument comment missing for literal argument 'flags' [bugprone-argument-comment] + // CHECK-MESSAGES-ALL: [[@LINE-3]]:10: warning: argument comment missing for literal argument 'size' [bugprone-argument-comment] + // CHECK-FIXES: f(/*flags=*/0x0, 4096, 1.0, true); + // CHECK-FIXES-ALL: f(/*flags=*/0x0, /*size=*/4096, 1.0, true); + + f(1, 4096, 0.0, true); + // CHECK-MESSAGES-ALL: [[@LINE-1]]:5: warning: argument comment missing for literal argument 'flags' [bugprone-argument-comment] + // CHECK-MESSAGES-ALL: [[@LINE-2]]:8: warning: argument comment missing for literal argument 'size' [bugprone-argument-comment] + // CHECK-FIXES-ALL: f(/*flags=*/1, /*size=*/4096, 0.0, true); + + f(~0, -0, 0.0, !0); + // CHECK-MESSAGES-ALL: [[@LINE-1]]:5: warning: argument comment missing for literal argument 'flags' [bugprone-argument-comment] + // CHECK-MESSAGES-ALL: [[@LINE-2]]:9: warning: argument comment missing for literal argument 'size' [bugprone-argument-comment] + // CHECK-MESSAGES-ALL: [[@LINE-3]]:18: warning: argument comment missing for literal argument 'force' [bugprone-argument-comment] + // CHECK-FIXES-ALL: f(/*flags=*/~0, /*size=*/-0, 0.0, /*force=*/!0); + + f(/*flags=*/0, /*size=*/0, 0.0, false); + f(ZERO, ZERO, 0.0, false); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
