[PATCH] D33537: [clang-tidy] Exception Escape Checker

[Dean Michael Berris via Phabricator via cfe-commits]

dberris added inline comments.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:154-155
+      if (const auto *TD = ThrownType->getAsTagDecl()) {
+        if (TD->getDeclName().isIdentifier() && TD->getName() == "bad_alloc")
+          return Results;
+      }
----------------
Does this actually catch `std::bad_alloc` or just any exception called `bad_alloc`? Should this be a fully qualified type, those defined by the implementation? I suspect this isn't as simple to figure out, because an implementation may be using nested namespaces (inline namespace?) to bring in the `bad_alloc` type in the `std` namespace.


================
Comment at: docs/ReleaseNotes.rst:230-233
+<<<<<<< HEAD
+=======
 - The 'google-runtime-member-string-references' check was removed.
+>>>>>>> master
----------------
You probably want to fix this...


================
Comment at: docs/clang-tidy/checks/bugprone-exception-escape.rst:28
+
+   Comma separated list containing function names which should not throw. An
+   example for using this parameter is the function ``WinMain()`` in the
----------------
baloghadamsoftware wrote:
> dberris wrote:
> > `EnabledFunctions` but they *should not* throw?
> Maybe we should come up with a better name for this option. I just took this from our company internal check.
My suggestion here would be something like 'FunctionBlacklist' or 'IgnoreFunctions'.

Are these exact names, or regular expressions? Should they be regular expressions? How about those that are in namespaces?

You might want to explore being able to configure this similar to the Sanitizer blacklist, which supports function name globs. I can imagine this immediately getting really brittle too.


================
Comment at: test/clang-tidy/bugprone-exception-escape.cpp:178
+void indirect_implicit() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'indirect_implicit' throws
+  implicit_int_thrower();
----------------
How deep does this go? Say we have a call to a function that's extern which doesn't have 'noexcept' nor a dynamic exception specifier -- do we assume that the call to an extern function may throw? Does that warn? What does the warning look like? Should it warn? How about when you call a function through a function pointer?

The documentation should cover these cases and/or more explicitly say in the warning that an exception may throw in a noexcept function (rather than just "function <...> throws").


https://reviews.llvm.org/D33537