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