[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfc5f14c5fe78: [clang-tidy] Ignore declarations in
bugprone-exception-escape (authored by PiotrZSL).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148462/new/
https://reviews.llvm.org/D148462
Files:
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -655,7 +655,6 @@
}
int indirectly_recursive(int n) noexcept;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in
function 'indirectly_recursive' which should not throw exceptions
int recursion_helper(int n) {
indirectly_recursive(n);
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -201,6 +201,10 @@
` check.
Global options of the same name should be used instead.
+- Improved :doc:`bugprone-exception-escape
+ ` to not emit warnings for
+ forward declarations of functions.
+
- Improved :doc:`bugprone-fold-init-type
` to handle iterators that do not
define `value_type` type aliases.
Index: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
@@ -52,7 +52,8 @@
void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
- functionDecl(anyOf(isNoThrow(), cxxDestructorDecl(),
+ functionDecl(isDefinition(),
+ anyOf(isNoThrow(), cxxDestructorDecl(),
cxxConstructorDecl(isMoveConstructor()),
cxxMethodDecl(isMoveAssignmentOperator()),
hasName("main"), hasName("swap"),
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -655,7 +655,6 @@
}
int indirectly_recursive(int n) noexcept;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'indirectly_recursive' which should not throw exceptions
int recursion_helper(int n) {
indirectly_recursive(n);
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -201,6 +201,10 @@
` check.
Global options of the same name should be used instead.
+- Improved :doc:`bugprone-exception-escape
+ ` to not emit warnings for
+ forward declarations of functions.
+
- Improved :doc:`bugprone-fold-init-type
` to handle iterators that do not
define `value_type` type aliases.
Index: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
@@ -52,7 +52,8 @@
void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
- functionDecl(anyOf(isNoThrow(), cxxDestructorDecl(),
+ functionDecl(isDefinition(),
+ anyOf(isNoThrow(), cxxDestructorDecl(),
cxxConstructorDecl(isMoveConstructor()),
cxxMethodDecl(isMoveAssignmentOperator()),
hasName("main"), hasName("swap"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
isuckatcs accepted this revision. isuckatcs added a comment. This revision is now accepted and ready to land. I think there's no point of holding back this patch. Even though I'm not 100% sure we want this change, I say let's merge it and see how our users react. It's a one line change anyway, so if we receive a lot of complaints, it will be easy to revert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148462/new/ https://reviews.llvm.org/D148462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
carlosgalvezp added a comment. I would agree that the fact that a function throws or not can only be found out when analyzing the function definition (it's impossible to know from the declaration). There's also a duplicate diagnostic that I don't see much value on, so I would agree with this patch unless I'm missing some concrete use case I'm not aware of. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148462/new/ https://reviews.llvm.org/D148462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
PiotrZSL added a comment. @njames93 What do you thing ? Should bugprone-exception-escape provide warnings for all forward declarations and definition, or only for definition. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148462/new/ https://reviews.llvm.org/D148462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
isuckatcs added a comment. > No because indirectly_recursive called from recursion_helper is noexcept, so > there will be std::terminate called. I missed that in `noexcept` functions thrown exceptions are not propagated. In this case I agree that `recursion_helper()` shouldn't emit a warning. As for the forward declaration part I think we should wait and see what others think about it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148462/new/ https://reviews.llvm.org/D148462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
PiotrZSL added a comment. @isuckatcs "Note that this particular warning is reported for the function and not for something inside the definition." Function declaration is not a function. A function declaration is a statement in programming languages that declares the existence of a function, including its name, parameters, and return type (if applicable). It is used to define the function and make it available for use in the program. On the other hand, a function is a set of instructions that performs a specific task and can be called by other parts of the program. When a function declaration is executed, it creates a function object that can be called as a function. So, while a function declaration is a necessary step in creating a function, it is not the function itself. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148462/new/ https://reviews.llvm.org/D148462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
PiotrZSL added a comment. @isuckatcs "Technically the exception is propagated through the function until a handler is found that catches it." No because indirectly_recursive called from recursion_helper is noexcept, so there will be std::terminate called. "Also we have cross translation unit analysis" Not in clang-tidy, and that work based on AST merge, so even if someone manage to run it here, it will work fine. "Please add a test case for this regardless of the behaviour to see that the checker handles exception propagation." There is test for that. The one with recursion_helper + indirectly_recursive. Be more specific if you want something else. "Because recursion_helper() propagates the thrown object it makes sense to emit a warning for that too." No it's not propagating thrown object. Bug: https://github.com/llvm/llvm-project/issues/54956 As for warnings for forward declaration, what developer have to do with such information ? There is nothing he can change in forward declaration to fix this issue. And putting 2 or more NOLINTS to silence single issue is stupid. Other checks validate only definitions. Forward declarations are forward declarations, they transparent for an exception propagation. And current behaviour introduce also performance penalty, as same thing is done multiple times. If you somehow want to point to developer issue about function declaration, there are notes for that, but still developer know about declarations, and to fix issue He/She don't need to touch this, only definition need to be changed (unlles decides to remove noexcept keyword, but then compiler will tell him about issues). Same goes for pure virtual functions, check does not emit warnings for them just because some implementation throw exception. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148462/new/ https://reviews.llvm.org/D148462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
isuckatcs added a comment.
> "The warning was emitted at every occurence of the function. It might be
> confusing if it's only emitted for the definition."
> Why ? Issue is in definition, not declaration.
For me it would be confusing, because the forward declaration is naming the
same function as the definition.
If I see that something is reported for the definition but not for the
declaration I might think it's a bug in the
tool, like once there is a problem with the function and the other time there
isn't. Note that this particular
warning is reported for the function and not for something inside the
definition.
Also we have cross translation unit analysis, though I'm not sure this
particular checker works there too.
Assuming it does, what happens if I forward declare the function in one
translation unit and define it in
a different one? With this change the warning will only be output in the
translation unit,the function is
defined in and this might silently hide some other problems in the TU the
function is forward declared in.
> recursion_helper does not throw, it crashes.
Technically the exception is propagated through the function until a handler is
found that catches it.
> Example with indirectly_recursive & recursion_helper behave in same way, only
> difference is that warning is emitted only for definition.
Please add a test case for this regardless of the behaviour to see that the
checker handles exception propagation.
> This is other bug that I'm fixing (not under this patch) related to properly
> handling noexcept keyword.
I'm not sure what you mean by bug here.
int recursion_helper(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in
// function 'recursion_helper' which should not throw exceptions
indirectly_recursive(n);
}
int indirectly_recursive(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in
// function 'indirectly_recursive' which should not throw exceptions
if (n == 0)
throw n;
return recursion_helper(n);
}
Because `recursion_helper()` propagates the thrown object it makes sense to
emit a warning for that too.
Also because the warning is emitted upon every propagation it is easy to trace
where the exception actually
comes from. Think of it like a stack trace for example.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148462/new/
https://reviews.llvm.org/D148462
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
PiotrZSL added a comment. @isuckatcs No it were not fine, check function were executed, ExceptionAnalyzer created only to bail out due to nullptr getBody for functions with only declaration. For functions with separate declaration and definition entire analysis is executed twice. simply because FunctionDecl::getBody jumps to function definition. And this simply isn't needed, as we analyse definition anyway, so lets just emit warning for definition. Now it forces developer to put NOLINT's into both .cpp and .hpp for same issue. "The warning was emitted at every occurence of the function. It might be confusing if it's only emitted for the definition." Why ? Issue is in definition, not declaration. Example with indirectly_recursive & recursion_helper behave in same way, only difference is that warning is emitted only for definition. "We know that indirectly_recursive(int n) throws when it shouldn't and that means recursion_helper(int n) will also throw when it shouldn't either." Well no indirectly_recursive throws when it shouldn't but only because `thrower` throws, recursion_helper does not throw, it crashes. This is other bug that I'm fixing (not under this patch) related to properly handling noexcept keyword. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148462/new/ https://reviews.llvm.org/D148462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
isuckatcs added a comment.
I think the original behaviour was fine. The warning was emitted at every
occurence of the function. It might be confusing if it's only emitted for the
definition.
Also what happens in the following scenario:
int indirectly_recursive(int n) noexcept;
int recursion_helper(int n) noexcept {
indirectly_recursive(n);
}
We know that `indirectly_recursive(int n)` throws when it shouldn't and that
means `recursion_helper(int n)` will also throw when it shouldn't either.
Is it reported properly with this change?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148462/new/
https://reviews.llvm.org/D148462
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D148462: [clang-tidy] Ignore declarations in bugprone-exception-escape
PiotrZSL created this revision.
PiotrZSL added reviewers: njames93, carlosgalvezp.
Herald added a subscriber: xazax.hun.
Herald added a project: All.
PiotrZSL requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Warnings will now only be printed for function definitions, not declarations
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D148462
Files:
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -651,7 +651,6 @@
}
int indirectly_recursive(int n) noexcept;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in
function 'indirectly_recursive' which should not throw exceptions
int recursion_helper(int n) {
indirectly_recursive(n);
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -191,6 +191,10 @@
` check.
Global options of the same name should be used instead.
+- Improved :doc:`bugprone-exception-escape
+ ` to not emit warnings for
+ forward declarations of functions.
+
- Improved :doc:`bugprone-fold-init-type
` to handle iterators that do not
define `value_type` type aliases.
Index: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
@@ -52,7 +52,8 @@
void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
- functionDecl(anyOf(isNoThrow(), cxxDestructorDecl(),
+ functionDecl(isDefinition(),
+ anyOf(isNoThrow(), cxxDestructorDecl(),
cxxConstructorDecl(isMoveConstructor()),
cxxMethodDecl(isMoveAssignmentOperator()),
hasName("main"), hasName("swap"),
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -651,7 +651,6 @@
}
int indirectly_recursive(int n) noexcept;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'indirectly_recursive' which should not throw exceptions
int recursion_helper(int n) {
indirectly_recursive(n);
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -191,6 +191,10 @@
` check.
Global options of the same name should be used instead.
+- Improved :doc:`bugprone-exception-escape
+ ` to not emit warnings for
+ forward declarations of functions.
+
- Improved :doc:`bugprone-fold-init-type
` to handle iterators that do not
define `value_type` type aliases.
Index: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
@@ -52,7 +52,8 @@
void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
- functionDecl(anyOf(isNoThrow(), cxxDestructorDecl(),
+ functionDecl(isDefinition(),
+ anyOf(isNoThrow(), cxxDestructorDecl(),
cxxConstructorDecl(isMoveConstructor()),
cxxMethodDecl(isMoveAssignmentOperator()),
hasName("main"), hasName("swap"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
