aaron.ballman added a comment. In https://reviews.llvm.org/D33333#761126, @jyu2 wrote:
> In https://reviews.llvm.org/D33333#760431, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D33333#760419, @jyu2 wrote: > > > > > In https://reviews.llvm.org/D33333#760149, @aaron.ballman wrote: > > > > > > > As an FYI, there is a related check currently under development in > > > > clang-tidy; we probably should not duplicate this functionality in both > > > > places. See https://reviews.llvm.org/D19201 for the other review. > > > > > > > > > To my understanding, clang-tidy is kind of source check tool. It does > > > more intensive checking than compiler. > > > My purpose here is to emit minimum warning form compiler, in case, > > > clang-tidy is not used. > > > > > > Sort of correct. clang-tidy doesn't necessarily do more intensive checking > > than the compiler. It's more an AST matching source checking tool for > > checks that are either expensive or have a higher false-positive rate than > > we'd want to see in the frontend. > > > > I think that this particular check should probably be in the frontend, like > > you're doing. My biggest concern is that we do not duplicate functionality > > between the two tools. https://reviews.llvm.org/D19201 has has a > > considerable amount of review, so you should look at the test cases there > > and ensure you are handling them (including the fixits). Hopefully your > > patch ends up covering all of the functionality in the clang-tidy patch. > > > > > In https://reviews.llvm.org/D33333#760353, @Prazek wrote: > > > > > >> Could you add similar tests as the ones that Stanislaw provied in his > > >> patch? > > >> Like the one with checking if throw is catched, or the conditional > > >> noexcept (by a macro, etc) > > > > > > > > > Good idea! Could add “marco” test for this. But I am not sure compiler > > > want to add check for throw caught by different catch handler. Because > > > at compile time, compiler can not statically determine which catch > > > handler will be used to caught the exception on all time. I would think > > > that is pragma's responsibility. > > > > > > For example: > > > > > > If (a) throw A else throw B; > > > > > > > > > > > > My main concern there is implicit noexcept gets set by compiler, which > > > cause runtime to termination. > > > As I said, I don't think checking throw type matching catch handler type is > compiler's job. I'd like either silence all warning for that. What do you > think? Consider the following cases: void f() noexcept { // Should not diagnose try { throw 12; } catch (int) { } } void g() noexcept { // Should not diagnose try { throw 12; } catch (...) { } } void h() noexcept { // Should diagnose try { throw 12; } catch (const char *) { } } void i() noexcept { // Should diagnose try { throw 12; } catch (int) { throw; } } void j() noexcept { // Should diagnose try { throw 12; } catch (int) { throw "haha"; } } void k() noexcept { // Should diagnose try { throw 12; } catch (...) { throw; } } I think the expected behavior in these cases is reasonable (and can be done with a CFG) rather than manually looking at the scope stack like you're doing. ================ Comment at: include/clang/Basic/DiagnosticGroups.td:140 def Exceptions : DiagGroup<"exceptions">; +def ThrowInNoexceptFunc : DiagGroup<"throw-in-noexcept-function">; ---------------- No need to add a group for this; you can define one inline with the warning, but perhaps the exceptions group already covers it. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6335 +def warn_throw_in_noexcept_func + : Warning<"'%0' function assumed not to throw an exception but does. " + "Throwing exception may cause termination.">, ---------------- Do not quote %0. Also, remove the full-stop and capitalized "Throwing". Diagnostics are not complete sentences. I think it should probably read `"%0 has a non-throwing exception specification but can still throw, resulting in unexpected program termination"` (or something along those lines). ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6342 +def note_throw_in_function + : Note<"__declspec(nothrow), throw(), noexcept(true), or noexcept was " + "specified on the function">; ---------------- Rather than make the note spell out all the various ways a function can be nothrow, we should either say "nonthrowing function declare here" or specify which reason the function is not throwing using %select. ================ Comment at: include/clang/Sema/Sema.h:4970 bool CheckCXXThrowOperand(SourceLocation ThrowLoc, QualType ThrowTy, Expr *E); + /// Check if throw is used in function with non-throwing noexcept + /// specifier. ---------------- with non-throwing -> with a non-throwing ================ Comment at: lib/Sema/SemaExprCXX.cpp:690 +static bool isNoexcept(const FunctionDecl * FD) +{ ---------------- Please run clang-format over the patch (the asterisk binds to `FD` and the curly brace should be on this line). ================ Comment at: lib/Sema/SemaExprCXX.cpp:692 +{ + if (const FunctionProtoType *FPT = FD->getType()->castAs<FunctionProtoType>()) + if (FPT->getExceptionSpecType() != EST_None && ---------------- Can use `const auto *` here. ================ Comment at: lib/Sema/SemaExprCXX.cpp:693-696 + if (FPT->getExceptionSpecType() != EST_None && + FPT->getNoexceptSpec(FD->getASTContext()) == + FunctionProtoType::NR_Nothrow) + return true; ---------------- Is there a reason for doing this rather than simply calling `FunctionProtoType::isNothrow()`? ================ Comment at: lib/Sema/SemaExprCXX.cpp:712 + bool isInCXXTryBlock = false; + for (auto *S = getCurScope(); S; S = S->getParent()) + if (S->getFlags() & (Scope::TryScope)) { ---------------- Please spell out the type explicitly here. ================ Comment at: lib/Sema/SemaExprCXX.cpp:713 + for (auto *S = getCurScope(); S; S = S->getParent()) + if (S->getFlags() & (Scope::TryScope)) { + isInCXXTryBlock = true; ---------------- Remove spurious parens. ================ Comment at: lib/Sema/SemaExprCXX.cpp:716 + break; + } else if (S->getFlags() & (Scope::FnScope)) + break; ---------------- Same here. ================ Comment at: test/CXX/except/except.spec/p11.cpp:5 // This is the "let the user shoot themselves in the foot" clause. -void f() noexcept { +void f() { throw 0; // no-error ---------------- Please modify this test to expect diagnostics rather than remove the `noexcept` from the test. This is testing specific features of the standard. ================ Comment at: test/SemaCXX/warn-throw-out-dtor.cpp:1 +// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions -fexceptions -fsyntax-only -Wthrow-in-noexcept-function -verify -std=c++11 +struct A { ---------------- Please remove the svn props from this file. https://reviews.llvm.org/D33333 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
