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

[Balázs Benics via Phabricator via llvm-commits]

steakhal added a comment.

Unfortunately, I'm not qualified enough to have much to say.



================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:1
+//===--- Env32CCheck.cpp - clang-tidy -------------------------------------===//
+//
----------------
Env32CCheck.cpp -> ExitHandlerCheck.cpp


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:38
+bool isExitFunction(StringRef FnName) {
+  return FnName == EF__Exit || FnName == EF_exit || FnName == EF_quick_exit;
+}
----------------
If `EF__Exit`, `EF_exit` and `EF_quick_exit` referred only once, we could simply inline those,

Same for the `isJumpFunction`.


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:60
+  /// Iteration over the collector is iteration over the found FunctionDecls.
+  auto begin() const -> decltype(CalledFunctions.begin()) {
+    return CalledFunctions.begin();
----------------
IMO `-> decltype(CalledFunctions.begin())` is unnecessary.
Return type deduction does the right thing for this case.

Same for the `auto end() ...`


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:84
+      hasArgument(
+        0, // the first argument is the handler function
+        declRefExpr(
----------------
If you disabled clang-format for having inline comments, Then you could create smaller matchers and give names for them.

Something similar to this:
```lang=cpp
const auto DesciptiveName = hasArgument(0, declRefExpr(hasDeclaration(functionDecl().bind("handler_decl"))));
```


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:132
+           "terminate by returning");
+      break;
+    }
----------------
Why don't we `return` here?
Same for the next `break`.


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:148-154
+    // Collect all the referenced FunctionDecls.
+    Collector.TraverseStmt(CurrentDefWithBody->getBody());
+    // Add called functions to the worklist.
+    std::copy(Collector.begin(), Collector.end(),
+              std::back_inserter(CalledFunctions));
+    // Reset the ASTVisitor instance results.
+    Collector.clear();
----------------
nit: I would clear the `Collector` before the `TraverseStmt`.


================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h:20
+/// Checker for SEI CERT rule ENV32-C
+/// All exit handlers must return normal.
+/// Exit handlers must terminate by returning. It is important and potentially
----------------
In the documentation you use the:
clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
> All exit handlers must return normally.

You should be consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717