[PATCH] D32350: [Analyzer] Exception checker for misuse: uncaught/noncompliant throws

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:105
+      ReportExnSpec(ReportExnSpec) {
+  // In the beginning we have to parse list formatted options
+  SmallVector<StringRef, 10> EnabledFuncsVec;
----------------
All comments should be full sentences starting with a capital letter and ending with a period. 


================
Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:109
+  EnabledFuncs.insert(EnabledFuncsVec.begin(), EnabledFuncsVec.end());
+  EnabledFuncs.insert("swap");
+  EnabledFuncs.insert("main");
----------------
whisperity wrote:
> Why is `swap` hardcoded as an "enabledfunc"?
It is always possible to implement swap in a non-throwing way, and some implementations that are using the copy and swap idiom, expecting swap to be no-throw. 


================
Comment at: test/Analysis/exception-misuse.cpp:26
+
+    ~Y() {  // expected-warning{{This function should not throw}}
+        // nesting
----------------
whisperity wrote:
> 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.
In fact,  the function can throw, if the exception is catched before leaving the function body. And in case the function does not throw but a called function do, that is also an error. So maybe something like exception are not allowed to leave this function? 


https://reviews.llvm.org/D32350