[PATCH] D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64
+/// argument.
+void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IsRegisterFunction =
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:68-71
+      0, declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")))
+             .bind("handler_expr"));
+  Finder->addMatcher(
+      callExpr(IsRegisterFunction, HasHandlerAsFirstArg).bind("register_call"),
----------------
It is customary in most Tidy checks that use multiple binds to have the bind names defined as a symbol, instead of using just two string literals as if the bind has to be renamed, it's easy to mess it up.


================
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",
----------------
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".)


================
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",
----------------
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"


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c:31
+// --------------
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
----------------
Which is the standard version this test file is set to analyse with? I don't see any `-std=` flag in the `RUN:` line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83717/new/

https://reviews.llvm.org/D83717