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";
+}
----------------
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).
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+ const auto IsRegisterFunction =
----------------
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
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:70
+ .bind("handler_expr"));
+ Finder->addMatcher(
+ callExpr(IsRegisterFunction, HasHandlerAsFirstArg).bind("register_call"),
----------------
I am not at all certain whether this is plausible, but I *think* this check can
be almost entirely implemented in the matchers rather than having to do manual
work. I think you can bind the argument node from the call to `at_quick_exit()`
and then use `equalsBoundNode()` to find the function calls within the bound
`functionDecl()` node.
However, if that's not workable, I think you can get rid of the
`CalledFunctionsCollector` entirely and just use matchers directly within the
`check()` function because by that point, you'll know exactly which AST nodes
you want to traverse through.
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:112-113
+ diag(RegisterCall->getBeginLoc(),
+ "exit-handler potentially calls an exit function. Handlers should "
+ "terminate by returning");
+ diag(HandlerDecl->getBeginLoc(), "handler function declared here",
----------------
whisperity wrote:
> Same as below, suggestion: "exit hander potentially calls exit function
> instead of terminating normally with a return".
>
> ("exit handler" and "exit function" without - is more in line with the SEI
> CERT rule's phrasing too, they don't say "exit-handler".)
Slight tweak to the suggestion: `exit hander potentially calls an exit function
instead of terminating normally with a return`
================
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",
----------------
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`?
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:128
+ DiagnosticIDs::Note);
+ diag(CurrentUsage->getBeginLoc(), "jump function called here",
+ DiagnosticIDs::Note);
----------------
If you make the suggested change above, I'd do a similar change here.
================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:1
+//===--- Env32CCheck.cpp - clang-tidy
-------------------------------------===//
+//
----------------
steakhal wrote:
> Env32CCheck.cpp -> ExitHandlerCheck.cpp
This comment was marked done but appears to still be an issue.
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
