[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D33825#2246369 <https://reviews.llvm.org/D33825#2246369>, @jfb wrote:

> MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. I'd much rather have a diagnostic which honors the state of things after http://wg21.link/p0270.

I agree, and I've commented on that rule to remind the folks at CERT that the rule has largely gone stale.

> Additionally, lots of what MSC54-CPP intends is implementation-defined. We shouldn't diagnose for things that clang has defined as signal safe, at least not by default.

Also agreed.



================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:42
+
+const char *cxxStmtText(const Stmt *stmt) {
+  if (llvm::isa<CXXBindTemporaryExpr>(stmt))
----------------
I'm not super keen on this approach because as new C++ constructs are added, this will continually need to be updated, which is a maintenance burden I think we should try to avoid. I would rather use some generic wording or tie the wording automatically to something provided by the `Stmt` class.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:44
+  if (llvm::isa<CXXBindTemporaryExpr>(stmt))
+    return "Binding temporary C++ expression here";
+  else if (llvm::isa<CXXBoolLiteralExpr>(stmt))
----------------
Diagnostics in clang-tidy should start with a lowercase letter


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:91
+const char *cxxDeclText(const Decl *decl) {
+  if (llvm::isa<CXXConstructorDecl>(decl))
+    return "Constructor declared here";
----------------
Similar concerns here.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D33825