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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits