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

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.


================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:54
+    // FIXME use DiagnosticIDs::Level::Note
+    diag(NoExceptRange.getBegin(), "in a function declared no-throw here:", DiagnosticIDs::Note)
+        << FixItHint::CreateRemoval(NoExceptRange);
----------------
alexfh wrote:
> Prazek wrote:
> > sbarzowski wrote:
> > > Prazek wrote:
> > > > sbarzowski wrote:
> > > > > alexfh wrote:
> > > > > > nit: `nothrow` (without a dash), no colon needed (it will look weird, since the location is mentioned _before_ the message, not after it)
> > > > > No, it's after the message now. When I changed the level to note the order of messages changed as well.
> > > > > 
> > > > > It looks like that:
> > > > > ```
> > > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:5:5: warning: 'throw' expression in a function declared with a non-throwing exception specification [misc-throw-with-noexcept]
> > > > >     throw 5;
> > > > >     ^
> > > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24: note: FIX-IT applied suggested code changes
> > > > > void f_throw_with_ne() noexcept(true) {
> > > > >                        ^
> > > > > /Users/uland/clang-new/build/tools/clang/tools/extra/test/clang-tidy/Output/misc-throw-with-noexcept.cpp.tmp.cpp:3:24: note: in a function declared nothrow here:
> > > > > void f_throw_with_ne() noexcept(true) {
> > > > >                        ^
> > > > > 
> > > > > ```
> > > > > 
> > > > > So, should I leave the colon or remove it anyway?
> > > > I think that the best way would be to have warnings in order:
> > > > 
> > > > warning function declared nothrow here have throw statement inside:
> > > > Note: throw statement here
> > > > 
> > > > Note: Fixit applied for every other declaration
> > > > 
> > > > What do you think Alex?
> > > > 
> > > > 
> > > > 
> > > @Prazek 
> > > So, do you suggest that we don't emit anything for additional declarations (without -fix)?
> > > 
> > > BTW (in the current version) I don't know if I can control if FIX-IT goes first or the location message. As you can see in the code the FIX-IT goes after the location.
> > Yep, there is no need for user to know all the locations if he doesn't want to perform fixit. This way it is easier to read the warning.
> The `FIX-IT applied suggested code changes` notes are shown by clang-tidy itself and only when the user passes -fix flag. There's no way to control them from the check (apart from removing fixes or attaching them to a different warning).
That's right, but does it make sense to show user all the places where the function was declared?
I am not sure what clang would normally do


================
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:
> 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?


https://reviews.llvm.org/D19201