aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33
+bool isExitFunction(StringRef FnName) {
+ return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit";
+}
----------------
njames93 wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Because this rule applies in C++ as well as C, you should protect these
> > > names so that calling something like this doesn't trigger the check:
> > > ```
> > > namespace menu_items {
> > > void exit(int);
> > > }
> > > ```
> > > I think we want `::_Exit` and `::std::_Exit` to check the fully qualified
> > > names (I believe this will still work in C, but should be tested). The
> > > same goes for the other names (including `atexit` and `at_quick_exit`
> > > above).
> > > I think we want `::_Exit` and `::std::_Exit` to check the fully qualified
> > > names (I believe this will still work in C, but should be tested).
> >
> > Tested with Clang-Query:
> >
> > ```
> > clang-query> m functionDecl(hasName("::atexit"))
> >
> > Match #1:
> >
> > /home/whisperity/LLVM/Build/foo.c:1:1: note: "root" binds here
> > int atexit(int) {}
> > ^~~~~~~~~~~~~~~~~~
> > 1 match.
> > ```
> That only works because the logic inside the `hasName`matcher
That's the bit I was worried about, too, thanks for confirming @njames93.
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+ const auto IsRegisterFunction =
----------------
whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > What happens if this test is run on C++ code calling the same functions?
> > > I see the rule is only applicable to C, for some reason... Should it be
> > > disabled from registering if by accident the check is enabled on a C++
> > > source file?
> > The CERT C++ rules inherit many of the C rules, including this one. It's
> > listed towards the bottom of the set of inherited rules here:
> > https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336
> Right, thanks for the heads-up. This should be somewhat more indicative on
> the Wiki on the page for the rule (especially because people won't
> immediately understand why a `-c` check reports on their cpp code, assuming
> they understand `-c` means C.)
I would imagine people shouldn't be confused by a C check triggering in C++
given that C++ incorporates much of C so there's a considerable amount of
overlap (for instance, this hasn't been an issue with `cert-env33-c` which
applies in both C and C++).
That said, what do you think should be indicated on the wiki (I assume you mean
the CERT wiki and not the clang-tidy documentation)? I'm happy to pass the
suggestion along to folks I still know at CERT.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83717/new/
https://reviews.llvm.org/D83717
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
