-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-tidy] Add a fully custom message to bugprone-unsafe-functions
#162443
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.
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Discookie (Discookie) ChangesIn some cases, such as when recommending the compiler option _FORTIFY_SOURCE, the current custom message format is clunky. Now, when the reason starts with
Full diff: https://github.com/llvm/llvm-project/pull/162443.diff 4 Files Affected:
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();
|
14adf28
to
42fea6d
Compare
✅ 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.
42fea6d
to
2bd457b
Compare
|
||
- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replacement string, when the Reason starts with the character `>` in the | |
replacement string, when the reason starts with the character `>` in the |
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