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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
