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