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

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

sbarzowski marked 3 inline comments as done.
sbarzowski added inline comments.


================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:25
+  Finder->addMatcher(
+      cxxThrowExpr(stmt(hasAncestor(functionDecl(isNoThrow()).bind("func"))))
+          .bind("throw"),
----------------
Prazek wrote:
> aaron.ballman wrote:
> > Prazek wrote:
> > > you can use forFunction instead of hasAncestor(functionDecl(
> > > cxxThrowExpr(stmt(forFunction(isNoThrow() or cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow())))
> > > you can even try to remove stmt.
> > > 
> > > 
> > > Also add test case
> > > 
> > >   void getFunction() noexcept {
> > >    struct X { 
> > >     static void inner()
> > >     {
> > >         throw 42;
> > >     }
> > >    }; 
> > >   }
> > > 
> > Hmm, I wonder if it's trivial enough to filter out throw statements that are inside a try block scope (even without checking the expression and catch block types)?
> unless(hasAncestor(cxxTryStmt())) should do the work for almost all cases. 
> Maybe even something like this to catch only valid try blocks
> 
>  cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow()).bind("func"))),
> unless(hasAncestor(cxxTryStmt().bind("try"), forFunction(hasBody(hasDescendant(equalsBoundNode("try")))))
> 
> + it should check that the throw is not in CXXCatchStmt (unless it is in another try block).
> 
Now even the caught type is checked.


================
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 {
----------------
aaron.ballman wrote:
> 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.
I have fixed these issues.


https://reviews.llvm.org/D19201