[PATCH] D33537: [clang-tidy] Exception Escape Checker

This revision was automatically updated to reflect the committed changes. Closed by commit rL336997: [clang-tidy] Exception Escape Checker (authored by baloghadamsoftware, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D33537: [clang-tidy] Exception Escape Checker

alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. Looks good with one comment. Comment at: test/clang-tidy/bugprone-exception-escape.cpp:178 +void indirect_implicit() noexcept { + // CHECK-MESSAGES: :[[@LINE-1]]:6:

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. Any news regarding this? https://reviews.llvm.org/D33537 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 151876. baloghadamsoftware added a comment. Typo fixed. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt clang-tidy/bugprone/ExceptionEscapeCheck.cpp

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added inline comments. Comment at: test/clang-tidy/bugprone-exception-escape.cpp:178 +void indirect_implicit() noexcept { + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_implicit' which should not throw

[PATCH] D33537: [clang-tidy] Exception Escape Checker

dberris added a comment. I'm really excited by this clang-tidy check and I think it's worth having. I do think there's more work needed to bring it up to a level of quality that would be helpful to more users. Comment at:

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 151666. baloghadamsoftware added a comment. New warning message, more detailed docs. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt

[PATCH] D33537: [clang-tidy] Exception Escape Checker

alexfh requested changes to this revision. alexfh added inline comments. This revision now requires changes to proceed. Comment at: docs/clang-tidy/checks/bugprone-exception-escape.rst:6 + +Finds functions which should not throw exceptions: +* Destructors I

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added inline comments. Comment at: test/clang-tidy/bugprone-exception-escape.cpp:178 +void indirect_implicit() noexcept { + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'indirect_implicit' throws + implicit_int_thrower(); lebedev.ri

[PATCH] D33537: [clang-tidy] Exception Escape Checker

lebedev.ri added inline comments. Comment at: test/clang-tidy/bugprone-exception-escape.cpp:178 +void indirect_implicit() noexcept { + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'indirect_implicit' throws + implicit_int_thrower(); baloghadamsoftware

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware marked 10 inline comments as done. baloghadamsoftware added inline comments. Comment at: test/clang-tidy/bugprone-exception-escape.cpp:178 +void indirect_implicit() noexcept { + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'indirect_implicit' throws

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 145452. baloghadamsoftware added a comment. Updated according to the comments. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt

[PATCH] D33537: [clang-tidy] Exception Escape Checker

dberris added inline comments. Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:154-155 + if (const auto *TD = ThrownType->getAsTagDecl()) { +if (TD->getDeclName().isIdentifier() && TD->getName() == "bad_alloc") + return Results; + }

[PATCH] D33537: [clang-tidy] Exception Escape Checker

alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed. Thank you for the updates. A few more comments. Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:150 + } else if (const auto *Try = dyn_cast(St)) { +

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 145174. baloghadamsoftware marked an inline comment as done. baloghadamsoftware added a comment. New test added. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: docs/clang-tidy/checks/bugprone-exception-escape.rst:8-9 +* Destructors +* Copy constructors +* Copy assignment operators +* The ``main()`` functions

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 145172. baloghadamsoftware added a comment. Typo fixed. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt clang-tidy/bugprone/ExceptionEscapeCheck.cpp

[PATCH] D33537: [clang-tidy] Exception Escape Checker

dberris added a comment. How about tests for functions with conditional noexcept? Comment at: docs/clang-tidy/checks/bugprone-exception-escape.rst:8-9 +* Destructors +* Copy constructors +* Copy assignment operators +* The ``main()`` functions Are copy

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. In https://reviews.llvm.org/D33537#1086571, @alexfh wrote: > It looks like you've missed some comments or uploaded a wrong patch. The latter. Now I retried to upload the correct patch. https://reviews.llvm.org/D33537

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 145155. baloghadamsoftware added a comment. Retrying... https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt clang-tidy/bugprone/ExceptionEscapeCheck.cpp

[PATCH] D33537: [clang-tidy] Exception Escape Checker

alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed. It looks like you've missed some comments or uploaded a wrong patch. Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:105 +const TypeVec

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 143068. baloghadamsoftware added a comment. Updated according to the comments. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. Thank you for your thorough review, I will do the fixes next week, after the Euro LLVM. https://reviews.llvm.org/D33537 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33537: [clang-tidy] Exception Escape Checker

alexfh requested changes to this revision. alexfh added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:100 + +namespace { + > make anonymous namespaces as small as possible, and only use

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 140117. baloghadamsoftware added a comment. ,html removed from release notes. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt clang-tidy/bugprone/ExceptionEscapeCheck.cpp

[PATCH] D33537: [clang-tidy] Exception Escape Checker

Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:68 +- New :doc:`bugprone-exception-escape + ` check + .html is not necessary. https://reviews.llvm.org/D33537 ___ cfe-commits mailing list

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware marked 4 inline comments as done. baloghadamsoftware added inline comments. Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:24-25 + +const TypeVec _throws(const FunctionDecl *Func); +const TypeVec _throws(const Stmt *St, const TypeVec ); +} //

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 140079. baloghadamsoftware added a comment. Updated according to the comments. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt

[PATCH] D33537: [clang-tidy] Exception Escape Checker

alexfh requested changes to this revision. alexfh added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:24-25 + +const TypeVec _throws(const FunctionDecl *Func); +const TypeVec _throws(const Stmt *St, const

[PATCH] D33537: [clang-tidy] Exception Escape Checker

Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:68 +- New :doc:`bugprone-exception-escape + ` check + Please fix link. See other checks as example.

[PATCH] D33537: [clang-tidy] Exception Escape Checker

alexfh added inline comments. Comment at: docs/clang-tidy/checks/misc-exception-escape.rst:1 +.. title:: clang-tidy - bugprone-exception-escape + This file should be renamed as well. https://reviews.llvm.org/D33537

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 139583. baloghadamsoftware added a comment. Moved to bugprone. https://reviews.llvm.org/D33537 Files: clang-tidy/bugprone/BugproneTidyModule.cpp clang-tidy/bugprone/CMakeLists.txt clang-tidy/bugprone/ExceptionEscapeCheck.cpp

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. Escaping exception is certainly a bug, so it should go into `bugprone` then. https://reviews.llvm.org/D33537 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33537: [clang-tidy] Exception Escape Checker

alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed. Herald added a subscriber: rnkovacs. Is there a more specific module for this check than misc? For example, does it check a rule that happens to appear in a certain coding standard?

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. We take the conservative approach there: if a function has no `throw` specifier we handle it as it would have a `noexcept` specifier. https://reviews.llvm.org/D33537 ___ cfe-commits mailing list

[PATCH] D33537: [clang-tidy] Exception Escape Checker

JVApen added a comment. Looking at the documentation and the examples, it is unclear to me what the behavior of checking is when you call a function without throw/noexcept if you can't see the implementation. (Other cpp file) https://reviews.llvm.org/D33537

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. In https://reviews.llvm.org/D33537#902242, @JonasToth wrote: > > Will this check find stuff like this (or is it part of the frontend) > > int throwing() { > throw int(42); > } > > int not_throwing() noexcept { > throwing(); > }

[PATCH] D33537: [clang-tidy] Exception Escape Checker

JonasToth added a comment. > I agree that we should not spend too much effort on making warnings from the > compiler and tidy disjunct. +1 There is an effort to treat clangs frontend warnings similar to clang-tidy checks within clang-tidy. This would allow to manage the overlap as well. Will

[PATCH] D33537: [clang-tidy] Exception Escape Checker

xazax.hun added a comment. I agree that we should not spend too much effort on making warnings from the compiler and tidy disjunct. https://reviews.llvm.org/D33537 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. I think that the acceptable false positive rate of a compiler warning is much lower than that of a Tidy check. So I understand that the fronted patch will not handle the indirect cases (which are the most important in my opinion) or the cases where there are

[PATCH] D33537: [clang-tidy] Exception Escape Checker

jyu2 added a comment. In https://reviews.llvm.org/D33537#827509, @baloghadamsoftware wrote: > Test changed. I made some bad throws reachable, but the frontend check still > does detects them. If block contains two or more throws, that mean compiler can not statically know throw is really

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 109126. baloghadamsoftware added a comment. Herald added a subscriber: JDevlieghere. Test changed. I made some bad throws reachable, but the frontend check still does detects them. https://reviews.llvm.org/D33537 Files:

[PATCH] D33537: [clang-tidy] Exception Escape Checker

jyu2 added a comment. In https://reviews.llvm.org/D33537#771274, @aaron.ballman wrote: > In https://reviews.llvm.org/D33537#771159, @baloghadamsoftware wrote: > > > In https://reviews.llvm.org/D33537#770264, @aaron.ballman wrote: > > > > > I think we should try to get as much of this

[PATCH] D33537: [clang-tidy] Exception Escape Checker

erichkeane added a comment. In https://reviews.llvm.org/D33537#801889, @baloghadamsoftware wrote: > In https://reviews.llvm.org/D33537#771274, @aaron.ballman wrote: > > > The check in https://reviews.llvm.org/D3 is using a CFG, not just > > checking direct throws. > > > I tested the latest

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. In https://reviews.llvm.org/D33537#771274, @aaron.ballman wrote: > The check in https://reviews.llvm.org/D3 is using a CFG, not just > checking direct throws. I tested the latest revision (the fronted patch already included) on my test file.

[PATCH] D33537: [clang-tidy] Exception Escape Checker

aaron.ballman added a comment. In https://reviews.llvm.org/D33537#771159, @baloghadamsoftware wrote: > In https://reviews.llvm.org/D33537#770264, @aaron.ballman wrote: > > > I think we should try to get as much of this functionality in > > https://reviews.llvm.org/D3 as possible; there is a

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. In https://reviews.llvm.org/D33537#770264, @aaron.ballman wrote: > I think we should try to get as much of this functionality in > https://reviews.llvm.org/D3 as possible; there is a considerable amount > of overlap between that functionality and this

[PATCH] D33537: [clang-tidy] Exception Escape Checker

aaron.ballman added a comment. In https://reviews.llvm.org/D33537#765445, @baloghadamsoftware wrote: > In https://reviews.llvm.org/D33537#764834, @Prazek wrote: > > > How is that compared to https://reviews.llvm.org/D19201 and the clang patch > > mentioned in this patch? > > > Actually, this

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 100382. baloghadamsoftware added a comment. Ignoring bad_alloc. https://reviews.llvm.org/D33537 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/ExceptionEscapeCheck.cpp clang-tidy/misc/ExceptionEscapeCheck.h

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 100374. baloghadamsoftware added a comment. Check added to the Release Notes. https://reviews.llvm.org/D33537 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/ExceptionEscapeCheck.cpp clang-tidy/misc/ExceptionEscapeCheck.h

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. In https://reviews.llvm.org/D33537#764834, @Prazek wrote: > How is that compared to https://reviews.llvm.org/D19201 and the clang patch > mentioned in this patch? Actually, this check does much more. It does not only check for noexcept (and throw())

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware updated this revision to Diff 100369. baloghadamsoftware added a comment. Docs fixed according to the comments. https://reviews.llvm.org/D33537 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/ExceptionEscapeCheck.cpp clang-tidy/misc/ExceptionEscapeCheck.h

[PATCH] D33537: [clang-tidy] Exception Escape Checker

Prazek added a comment. How is that compared to https://reviews.llvm.org/D19201 and the clang patch mentioned in this patch? Repository: rL LLVM https://reviews.llvm.org/D33537 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33537: [clang-tidy] Exception Escape Checker

Eugene.Zelenko added a comment. Please mention this check in docs/ReleaseNotes.rst (in alphabetical order). Comment at: docs/clang-tidy/checks/misc-exception-escape.rst:7 +Finds functions which should not throw exceptions: ++ Destructors ++ Copy constructors I

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware added a comment. Supersedes https://reviews.llvm.org/D32350 https://reviews.llvm.org/D33537 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33537: [clang-tidy] Exception Escape Checker

baloghadamsoftware created this revision. Herald added a subscriber: mgorny. Finds functions which should not throw exceptions: Destructors, move constructors, move assignment operators, the main() function, swap() functions, functions marked with throw() or noexcept and functions given as