whisperity 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";
+}
----------------
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.
```
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+ const auto IsRegisterFunction =
----------------
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.)
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:124-125
+ diag(RegisterCall->getSourceRange().getBegin(),
+ "exit-handler potentially calls a jump function. Handlers should "
+ "terminate by returning");
+ diag(HandlerDecl->getBeginLoc(), "handler function declared here",
----------------
aaron.ballman wrote:
> whisperity wrote:
> > This semi-sentence structure of starting with lowercase but also
> > terminating the sentence and leaving in another but unterminated sentences
> > looks really odd.
> >
> > Suggestion: "exit handler potentially jumps instead of terminating normally
> > with a return"
> Slight tweak here as well. I don't think we'll ever see a jump function other
> than longjmp for this rule, so what about writing `potentially calls
> 'longjmp'` instead of `potentially jumps`?
😉 Agree. And if we see, we will have to update the code anyways. One could in
theory whip up some inline assembly black magic, but that's a whole other can
of worms.
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
