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