Skip to content

Conversation

@Discookie
Copy link
Contributor

In some cases, such as when recommending the compiler option _FORTIFY_SOURCE, the current custom message format is clunky. Now, when the reason starts with >, the replacement string is omitted., so only the Reason is shown.

^function$,,has a custom message; - function 'function' has a custom message; it should not be used
^function$,,>has a custom message and no replacement suggestion; - function 'function' has a custom message and no replacement suggestion

@llvmbot
Copy link
Member

llvmbot commented Oct 8, 2025

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Discookie (Discookie)

Changes

In some cases, such as when recommending the compiler option _FORTIFY_SOURCE, the current custom message format is clunky. Now, when the reason starts with >, the replacement string is omitted., so only the Reason is shown.

^function$,,has a custom message; - function 'function' has a custom message; it should not be used
^function$,,>has a custom message and no replacement suggestion; - function 'function' has a custom message and no replacement suggestion


Full diff: https://github.com/llvm/llvm-project/pull/162443.diff

4 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp (+7-1)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst (+5-1)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom.c (+3-3)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
index 0399af2a673f4..2470ea9681e9e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
@@ -304,11 +304,17 @@ void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
         StringRef Reason =
             Entry.Reason.empty() ? "is marked as unsafe" : Entry.Reason.c_str();
 
-        if (Entry.Replacement.empty()) {
+        // Omit the replacement, when a fully-custom reason is given.
+        if (Reason.consume_front(">")) {
+          diag(SourceExpr->getExprLoc(), "function %0 %1")
+               << FuncDecl << Reason.trim() << SourceExpr->getSourceRange();
+        // Do not recommend a replacement when it is not present.
+        } else if (Entry.Replacement.empty()) {
           diag(SourceExpr->getExprLoc(),
                "function %0 %1; it should not be used")
               << FuncDecl << Reason << Entry.Replacement
               << SourceExpr->getSourceRange();
+        // Otherwise, emit the replacement.
         } else {
           diag(SourceExpr->getExprLoc(),
                "function %0 %1; '%2' should be used instead")
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d13f83079e591..c876aaf1d182a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -270,6 +270,11 @@ Changes in existing checks
   an additional matcher that generalizes the copy-and-swap idiom pattern
   detection.
 
+- Improved :doc:`bugprone-unsafe-functions
+  <clang-tidy/checks/bugprone/unsafe-functions>` check by omitting the custom 
+  replacement string, when the Reason starts with the character `>` in the
+  `CustomFunctions` option.
+
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
   avoid false positives on inherited members in class templates.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst
index 317db9c5564e2..8945d3e91e631 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst
@@ -98,7 +98,9 @@ The `reason` is optional and is used to provide additional information about the
 reasoning behind the replacement. The default reason is `is marked as unsafe`.
 
 If `replacement` is empty, the text `it should not be used` will be shown
-instead of the suggestion for a replacement.
+instead of the suggestion for a replacement. If the `reason` starts with the
+character `>`, the replacement message is disabled, to allow better control over
+the suggestions.
 
 As an example, the configuration `^original$, replacement, is deprecated;`
 will produce the following diagnostic message.
@@ -114,6 +116,8 @@ qualified name (i.e. ``std::original``), otherwise the regex is matched against
 If the regular expression starts with `::` (or `^::`), it is matched against the
 fully qualified name (``::std::original``).
 
+The same diagnostic message is produced by the configuration `^original$,,> is deprecated; 'replacement' should be used instead`
+
 .. note::
 
    Fully qualified names can contain template parameters on certain C++ classes, but not on C++ functions.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom.c
index 7fd71ec2f2e7b..7eaf015f06aa2 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom.c
@@ -1,5 +1,5 @@
 // RUN: %check_clang_tidy -check-suffix=NON-STRICT-REGEX %s bugprone-unsafe-functions %t --\
-// RUN:   -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '::name_match,replacement,is a qualname match;^::prefix_match,,is matched on qualname prefix'}}"
+// RUN:   -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: \"::name_match,,>is a qualname match, but with a fully 'custom' message;^::prefix_match,,is matched on qualname prefix\"}}"
 // RUN: %check_clang_tidy -check-suffix=STRICT-REGEX     %s bugprone-unsafe-functions %t --\
 // RUN:   -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '^name_match$,replacement,is matched on function name only;^::prefix_match$,,is a full qualname match'}}"
 
@@ -11,14 +11,14 @@ void prefix_match_regex();
 
 void f1() {
   name_match();
-  // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match' is a qualname match; 'replacement' should be used instead
+  // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match' is a qualname match, but with a fully 'custom' message
   // CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'name_match' is matched on function name only; 'replacement' should be used instead
   prefix_match();
   // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'prefix_match' is matched on qualname prefix; it should not be used
   // CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'prefix_match' is a full qualname match; it should not be used
 
   name_match_regex();
-  // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match_regex' is a qualname match; 'replacement' should be used instead
+  // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'name_match_regex' is a qualname match, but with a fully 'custom' message
   // no-warning STRICT-REGEX
 
   prefix_match_regex();

@github-actions
Copy link

github-actions bot commented Oct 8, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

In some cases, such as when recommending the compiler option _FORTIFY_SOURCE, the current custom message format is clunky.
Now, when the reason starts with `>`, the replacement string is omitted., so only the Reason is shown.
Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, thanks for the commit!

Why did you select the prefix > for activating this feature? (It's perfectly acceptable, I'm just curious.)

I feel that it may be a bit confusing that in the comments / release notes etc. "replacement message" may also refer to the it should not be used suffix of the message. Maybe "default message suffix" or something similar would be less ambiguous.

@Discookie
Copy link
Contributor Author

Discookie commented Oct 16, 2025

No particular reason for choosing the prefix >. I thought about enclosing it in " quotes, with escape sequences like \", but that would require implementing a configuration parser in clang-tidy, and that seemed to be unnecessarily big in scope. A single-character prefix seemed better than that.

I'll rephrase the docs a bit to make it clearer.

@Discookie
Copy link
Contributor Author

Rewrote the docs, they are much clearer now.

Copy link
Contributor

@vbvictor vbvictor left a comment

Choose a reason for hiding this comment

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

LGTM with nit

memcpy(dest, src, 999'999); // warning: function 'memcpy' is recommended to have compiler hardening using '_FORTIFY_SOURCE'
printf(raw_str); // warning: function 'printf' is recommended to have the '-Werror=format-security' compiler warning flag
The
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be removed?

Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

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

The content LGTM, but there is incorrect markup in the RST files.

Comment on lines +106 to +107
the suggestions. The starting `>` character and the preceding spaces are trimmed
from the message.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "preceding spaces" here? Spaces preceding the ">" mark or spaces between the ">" and the message?

Comment on lines +73 to +76
- Modified the custom message format of :doc:`bugprone-unsafe-functions
<clang-tidy/checks/bugprone/unsafe-functions>` by hiding the default suffix
when the reason starts with the character `>` in the `CustomFunctions` option.
The warning locations are not changed, but the message is different.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps highlight that this only changes the behavior in the unlikely case when the configured reason already started with >.


- Modified the custom message format of :doc:`bugprone-unsafe-functions
<clang-tidy/checks/bugprone/unsafe-functions>` by hiding the default suffix
when the reason starts with the character `>` in the `CustomFunctions` option.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere ensure that code fragments are surrounded by doubled backtick characters (two before the code fragment + two after it) because in RST single backticks creates links and you need doubled backticks to use monospace font.

Note that this differs from markdown (e.g. github comments) where monospace (code) text is marked by single backticks (while links use a completely different format: [text](url)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants