[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

[Stanisław Barzowski via Phabricator via cfe-commits]

sbarzowski added inline comments.


================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {
----------------
sbarzowski wrote:
> vmiklos wrote:
> > Prazek wrote:
> > > aaron.ballman wrote:
> > > > What is the false positive rate with this check over a large codebase that uses exceptions?
> > > Do you know any good project to check it?
> > LibreOffice might be a candidate, see <https://wiki.documentfoundation.org/Development/ClangTidy> for details on how to generate a compile database for it (since it does not use cmake), then you can start testing.
> Thanks. However it's not just about using exception. To be meaningful I need a project that use noexcept in more than a few places.
> 
> Here are some projects that I found:
> https://github.com/facebook/hhvm/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/facebook/folly/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/Tencent/mars/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/facebook/rocksdb/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/CRYTEK-CRYENGINE/CRYENGINE/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/SFTtech/openage/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/facebook/watchman/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/facebook/proxygen/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/philsquared/Catch/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/sandstorm-io/capnproto/search?utf8=%E2%9C%93&q=noexcept
> https://github.com/miloyip/rapidjson/search?utf8=%E2%9C%93&q=noexcept
> 
> I've tried HHVM so far, and ran into some problems compiling it. Anyway I'm working on it.
Ok, I was able to run it on most of the HHVM  codebase + deps. There were some issues that looked like some autogenerated pieces missing, so it may have been not 100% covered.

The results:
3 occurences in total
1) rethrow in destructor (http://pastebin.com/JRNMZGev)
2) false positive "throw and catch in the same function" (http://pastebin.com/14y1AJgM)
3) noexcept decided during compilation (http://pastebin.com/1jZzRAbC)

That's not too many, but this is a kind of check that I guess would be most useful earlier in the development - ideally before the initial code review.

I'm not sure if we should count (3) as false positive. We could potentially eliminate it, by checking if the expression in noexcept is empty or true literal.

(2) is def. a false positive. The potential handling of this case was already proposed, but I'm not sure if it's worth it.  The code in example (2) is ugly and extracting these throwing parts to separate functions looks like a good way to start refactoring. 

What do you think?



https://reviews.llvm.org/D19201