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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Just one question if you could try this out for me: what happens if you run `clang-tidy a.c b.c` (two TUs in the invocation) where **one of them** (preferably the later one, i.e. **`b.c`**) does //NOT// have Annex K enabled? I believe the cached `IsAnnexKAvailable` (like any other TU-specific state of the check instance) should be invalidated/cleared in an overridden `void onStartTranslationUnit()` function.

Also, what happens if the check is run for C++ code?



================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:208-214
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    auto &Opts = Options.CheckOptions;
+    Opts["bugprone-unsafe-functions.ReportMoreUnsafeFunctions"] = "true";
+    return Options;
+  }
----------------
What is the reason for this being a //module// option, when the name of the option looks like a //check// option? Did something change and the API requires this now? If you do `Options.get("ReportMoreUnsafeFunctions", true)` it will automatically work and use this default option. You also overridden the `storeOptions()` function properly by the looks of it, so there should be no reason for this change.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:157-159
+
+  if (!getLangOpts().C11) {
+    // Caching the result.
----------------
(And you can remove all the other `// Caching the result` comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:203-212
+  Optional<StringRef> AnnexKReplacementFunction =
+      useSafeFunctionsFromAnnexK()
+          ? StringSwitch<Optional<StringRef>>(FunctionName)
+                .Cases("asctime", "asctime_r", Optional<StringRef>{"asctime_s"})
+                .Case("gets", Optional<StringRef>{"gets_s"})
+                .Default(None)
+          : None;
----------------
Instead of wrapping the `StringRef` into an `Optional`, couldn't we achieve the same with the empty string(ref)... signalling the fact that there is no replacement?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h:21
+/// deprecated or missing bounds checking. For the listed functions a
+/// replacement function is suggested. The checker heavily relies on the
+/// functions from Annex K (Bounds-checking interfaces) of C11.
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h:47
+
+  // Checker option.
+  const bool ReportMoreUnsafeFunctions;
----------------
(Superfluous comment.)


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:125
+  deprecated or missing bounds checking. For the listed functions a
+  replacement function is suggested. The checker heavily relies on the
+  functions from Annex K (Bounds-checking interfaces) of C11.
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unsafe-functions.rst:9
+deprecated or missing bounds checking. For the listed functions a
+replacement function is suggested. The checker heavily relies on the
+functions from Annex K (Bounds-checking interfaces) of C11.
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unsafe-functions.rst:81-83
+    strcpy(buf, prefix); // warning: function 'strcpy' is not bounds-checking; 'strcpy_s' should be used instead.
+    strcat(buf, msg); // warning: function 'strcat' is not bounds-checking; 'strcat_s' should be used instead.
+    strcat(buf, suffix); // warning: function 'strcat' is not bounds-checking; 'strcat_s' should be used instead.
----------------
(Visual nit.)


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

https://reviews.llvm.org/D91000