[PATCH] D32350: [Analyzer] Exception Checker

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:80
+  bool WasReThrow;
+  Types CurrentThrows; // actually alive exceptions
+  FunctionExceptionsMap
----------------
There are some comment formatting issues along these lines.


================
Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:84
+  std::vector<std::string> CallStack;        // trace call hierarchy
+  SmallSet<StringRef, 10> EnabledFuncs;      // allowed functions
+  SmallSet<StringRef, 10> IgnoredExceptions; // ignored exceptions
----------------
I had to stop here for a moment and heavily think what this variable (and the relevant command-line argument) is used for.

Maybe this calls for a comment then. What is "allowed function"? One that is **explicitly** allowed to throw, based on the user's decision? This should be explained here.


================
Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:109
+  EnabledFuncs.insert(EnabledFuncsVec.begin(), EnabledFuncsVec.end());
+  EnabledFuncs.insert("swap");
+  EnabledFuncs.insert("main");
----------------
Why is `swap` hardcoded as an "enabledfunc"?


================
Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:329
+  const FunctionDecl *callee = C->getDirectCallee();
+  // if is not exist body for this function we do not care about
+  if (!callee || !callee->hasBody()) {
----------------
The phrasing should be fixed here for easier understanding.


================
Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:336
+  FunctionExceptionsMap::const_iterator fnIt = ExceptionsThrown.find(name);
+  // already processed?
+  if (fnIt == ExceptionsThrown.end()) {
----------------
`already processed` what? A given exception type from a given function?


================
Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:468
+  const auto *D = pNode.get<Decl>();
+  if (D == nullptr)
+    return false;
----------------
`if (!D)`


================
Comment at: test/Analysis/exception-misuse.cpp:18
+
+    Y(Y&&) { // expected-warning{{This function should not throw}}
+        throw data;
----------------
I would use a much more descriptive error message here. E.g., explicitly say, that `move (constructor|operator=) should not throw`.


================
Comment at: test/Analysis/exception-misuse.cpp:26
+
+    ~Y() {  // expected-warning{{This function should not throw}}
+        // nesting
----------------
Yet again, better wording: _Destructor not marked `noexcept(false)` should not throw_ (this is true since `C++11`, maybe this needs to be based on a conditional in the checker!)

@xazax.hun, any idea on what a good error message here should be?


================
Comment at: test/Analysis/exception-misuse.cpp:26
+
+    ~Y() {  // expected-warning{{This function should not throw}}
+        // nesting
----------------
whisperity wrote:
> Yet again, better wording: _Destructor not marked `noexcept(false)` should not throw_ (this is true since `C++11`, maybe this needs to be based on a conditional in the checker!)
> 
> @xazax.hun, any idea on what a good error message here should be?
Also, a test case for a throwing, and `noexcept(false)`-specified dtor is missing.


https://reviews.llvm.org/D32350