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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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:
> 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?
> 
The fact that there's a false positive in the first large project you checked suggests that the false positive is likely worth it to fix. The code may be ugly, but it's not uncommon -- some coding guidelines explicitly disallow use of gotos, and this is a reasonable(ish) workaround for that.

As for #3, I would consider that to be a false-positive as well. A computed noexcept clause is going to be very common, especially in generic code. I think it's probably more important to get this one right than #2.


https://reviews.llvm.org/D19201