[PATCH] D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:19
+namespace {
+static Preprocessor *PP;
+}
----------------
Why this could not be member of check class?


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:52
+  bool AnnexKIsWanted;
+  if (!PP->isMacroDefined("__STDC_LIB_EXT1__")) {
+    AnnexKIsAvailable = false;
----------------
Why not to use plain assignment? See `readability-simplify-boolean-expr`.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:59
+  AnnexKIsWanted = false;
+  if (!Id) {
+    AnnexKIsWanted = false;
----------------
Early return will be better.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:69
+    AreSafeFunctionsWanted = IntValue.getZExtValue();
+    if (AreSafeFunctionsWanted.hasValue()) {
+      AnnexKIsWanted = AreSafeFunctionsWanted.getValue();
----------------
Please elide braces.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:80
+  bool AnnexKIsWanted = std::get<1>(AnnexKUsage);
+  if (!AnnexKIsWanted || !AnnexKIsAvailable) {
+    return;
----------------
Please elide braces.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:84
+  if (const auto *Function = Result.Nodes.getNodeAs<CallExpr>("callexpr")) {
+    const FunctionDecl *FD = Result.Nodes.getNodeAs<FunctionDecl>("fn");
+    const NamedDecl *NFD = dyn_cast<NamedDecl>(FD);
----------------
Could be `const auto *`, because type is spelled in same statement.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:85
+    const FunctionDecl *FD = Result.Nodes.getNodeAs<FunctionDecl>("fn");
+    const NamedDecl *NFD = dyn_cast<NamedDecl>(FD);
+    std::string FunctionName = NFD->getNameAsString();
----------------
Could be `const auto *`, because type is spelled in same statement.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:92
+  if (const auto *FunctionPointer = Result.Nodes.getNodeAs<VarDecl>("fp")) {
+    if (const FunctionDecl *FD = Result.Nodes.getNodeAs<FunctionDecl>("fnp")) {
+      const NamedDecl *NFD = dyn_cast<NamedDecl>(FD);
----------------
Could be `const auto *`, because type is spelled in same statement.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.cpp:93
+    if (const FunctionDecl *FD = Result.Nodes.getNodeAs<FunctionDecl>("fnp")) {
+      const NamedDecl *NFD = dyn_cast<NamedDecl>(FD);
+      std::string FunctionName = NFD->getNameAsString();
----------------
Could be `const auto *`, because type is spelled in same statement.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h:12
+
+#include "../ClangTidyCheck.h"
+namespace clang {
----------------
Please separate include statement and namespaces with empty line.


================
Comment at: clang-tools-extra/clang-tidy/cert/ObsolescentFunctionsCheck.h:13
+#include "../ClangTidyCheck.h"
+namespace clang {
+namespace tidy {
----------------
Please separate with empty line.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst:6
+    
+Guards against using some unsafe function calls and function pointers which initialized with unsafe functions if __STDC_LIB_EXT1__ macro is defined and the value of __STDC_WANT_LIB_EXT1__ is 1. The usage of following functions are checked : bsearch,
+    fprintf, fscanf, fwprintf, fwscanf, getenv, gmtime, localtime, mbsrtowcs,
----------------
Please make first statement same as in Release Notes. Please enclose all preprocessor variables and function names in double back-ticks.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst:15
+
+This is a CERT security rule:
+https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions
----------------
Link to original rule should be at the end. See other documentation as example. 


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst:17
+https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions
+      Example :
+      .code-block::
----------------
Please remove space before colon and separate code with empty line.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-obsolescent-functions.rst:18
+      Example :
+      .code-block::
+        #define __STDC_WANT_LIB_EXT1__ 1
----------------
Should be `.. code-block:: c++`. See other documentation as formatting example.


================
Comment at: clang-tools-extra/test/clang-tidy/cert-obsolescent-functions.cpp:5
+#define __STDC_WANT_LIB_EXT1__ 1
+void * memmove(void *, void *, unsigned int);
+void f1(const char *in) {
----------------
Please separate with empty line.


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

https://reviews.llvm.org/D91000