Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
10 changes: 10 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ Potentially Breaking Changes
- `CharTypdefsToIgnore` to `CharTypedefsToIgnore` in
:doc:`bugprone-signed-char-misuse
<clang-tidy/checks/bugprone/signed-char-misuse>`

- 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)).

The warning locations are not changed, but the message is different.
Comment on lines +73 to +76
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 >.


Improvements to clangd
----------------------
Expand Down Expand Up @@ -314,6 +319,11 @@ Changes in existing checks
<clang-tidy/checks/bugprone/unhandled-self-assignment>` check by adding
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 hiding the default
suffix when the reason starts with the character `>` in the `CustomFunctions`
option.

- Improved :doc:`cppcoreguidelines-init-variables
<clang-tidy/checks/cppcoreguidelines/init-variables>` check by fixing the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ including any system headers.
Custom functions
----------------

The option :option:`CustomFunctions` allows the user to define custom functions to be
checked. The format is the following, without newlines:
The option :option:`CustomFunctions` allows the user to define custom functions
to be checked. The format is the following, without newlines:

.. code::

Expand All @@ -97,33 +97,61 @@ The functions are matched using POSIX extended regular expressions.
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.
If `replacement` is empty, the default text `it should not be used` will be
shown instead of the suggestion for a replacement.

As an example, the configuration `^original$, replacement, is deprecated;`
will produce the following diagnostic message.
If the `reason` starts with the character `>`, the reason becomes fully custom.
The default suffix is disabled even if a `replacement` is present, and only the
reason message is shown after the matched function, to allow better control over
the suggestions. The starting `>` character and the preceding spaces are trimmed
from the message.
Comment on lines +106 to +107
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?


As an example, the following configuration matches only the function `original`
in the default namespace. A similar diagnostic can also be printed using a fully
custom reason.

.. code:: c

// bugprone-unsafe-functions.CustomFunctions:
// ^original$, replacement, is deprecated;
// Using the fully custom message syntax:
// ^original$,,> is deprecated, 'replacement' should be used instead;

original(); // warning: function 'original' is deprecated; 'replacement' should be used instead.
original(); // warning: function 'original' is deprecated; 'replacement' should be used instead
::std::original(); // no-warning
original_function(); // no-warning

If the regular expression contains the character `:`, it is matched against the
qualified name (i.e. ``std::original``), otherwise the regex is matched against the unqualified name (``original``).
If the regular expression starts with `::` (or `^::`), it is matched against the
fully qualified name (``::std::original``).
qualified name (i.e. ``std::original``), otherwise the regex is matched against
the unqualified name (``original``). If the regular expression starts with `::`
(or `^::`), it is matched against the fully qualified name
(``::std::original``).

One of the use cases for fully custom messages is suggesting compiler options
and warning flags:

.. code:: c

// bugprone-unsafe-functions.CustomFunctions:
// ^memcpy$,,>is recommended to have compiler hardening using '_FORTIFY_SOURCE';
// ^printf$,,>is recommended to have the '-Werror=format-security' compiler warning flag;

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?


.. note::

Fully qualified names can contain template parameters on certain C++ classes, but not on C++ functions.
Type aliases are resolved before matching.
Fully qualified names can contain template parameters on certain C++ classes,
but not on C++ functions. Type aliases are resolved before matching.

As an example, the member function ``open`` in the class ``std::ifstream``
has a fully qualified name of ``::std::basic_ifstream<char>::open``.

The example could also be matched with the regex ``::std::basic_ifstream<[^>]*>::open``, which matches all potential
template parameters, but does not match nested template classes.
The example could also be matched with the regex
``::std::basic_ifstream<[^>]*>::open``, which matches all potential template
parameters, but does not match nested template classes.

Options
-------
Expand Down
Original file line number Diff line number Diff line change
@@ -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'}}"

Expand All @@ -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();
Expand Down