[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Generally LGTM. Please revisit the documentation and let's fix the style, and then we can land.

In D91000#3197851 <https://reviews.llvm.org/D91000#3197851>, @aaron.ballman wrote:

> In terms of whether we should expose the check in C++: I think we shouldn't. [...]
>
> I think we should probably also not enable the check when the user compiles in C99 or earlier mode, because there is no Annex K available to provide replacement functions.

@aaron.ballman I think the current version of the check satisfies these conditions. It seems the check **will** run for every TU, but in case there is no alternative the check could suggest, it will do nothing:

  if (!ReplacementFunctionName)
    return;

Is this good enough? This seems more future-proof than juggling the `LangOpts` instance in yet another member function.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:1
+//===--- UnsafeFunctionsCheck.cpp - clang-tidy -----------------------===//
+//
----------------
Ditto.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:150
+                                               Preprocessor *PP,
+                                               Preprocessor *ModuleExpanderPP) {
+  this->PP = PP;
----------------
(Just so that we do not get "unused variable" warnings when compiling.)


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h:1
+//===--- UnsafeFunctionsCheck.h - clang-tidy ---------------*- C++ -*-===//
+//
----------------
Style nit: length of line is not padded to 80-col.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst:31-40
+The following functions are also checked (regardless of Annex K availability):
+ - `rewind`, suggested replacement: `fseek`
+ - `setbuf`, suggested replacement: `setvbuf`
+
+Based on the ReportMoreUnsafeFunctions option, the following functions are also checked:
+ - `bcmp`, suggested replacement: `memcmp`
+ - `bcopy`, suggested replacement: `memcpy_s` if Annex K is available, or `memcopy` if Annex K is not available
----------------
In general, the **rendered version** of the docs should be **visually** observed, as backtick in RST only turns on emphasis mode and not monospacing, i.e. it will be //memcpy_s// and not `memcpy_s`. Please look at the other checks and if there is a consistent style (e.g. symbol names (function) are monospaced, but options of the check is emphasised) align it to that so we have the documentation "fall into place" visually.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst:52
+Both macros have to be defined to suggest replacement functions from Annex K. `__STDC_LIB_EXT1__` is
+defined by the library implementation, and `__STDC_WANT_LIB_EXT1__` must be defined to "1" by the user
+before including any system headers.
----------------
(Style nit: yeah this will not look like a symbol at all here)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91000/new/

https://reviews.llvm.org/D91000