[PATCH] D18961: Add a readability-deleted-default clang-tidy check.

aaron.ballman added a comment. In D18961#2549303 , @njames93 wrote: > Now that there is a stable warning in clang for this check that is enabled > without specifying any warning flags, Is there reason to think about retiring > this check? I'd be fine wi

[PATCH] D18961: Add a readability-deleted-default clang-tidy check.

njames93 added a comment. Herald added a subscriber: mgorny. Now that there is a stable warning in clang for this check that is enabled without specifying any warning flags, Is there reason to think about retiring this check? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.l

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

This revision was automatically updated to reflect the committed changes. Closed by commit rL266190: [clang-tidy] Add a readability-deleted-default clang-tidy check. (authored by alexfh). Changed prior to commit: http://reviews.llvm.org/D18961?vs=53533&id=53536#toc Repository: rL LLVM http:

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added a comment. Sorry, both my 'diff' and my 'svn' command added some magical coloring... It should be good now. http://reviews.llvm.org/D18961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/lis

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53533. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/ReadabilityTidyModule.cpp docs/ReleaseNotes.rst doc

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53532. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/ReadabilityTidyModule.cpp docs/ReleaseNotes.rst

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added a comment. Something is weird with your patch: it has names starting with a space (note two spaces between "Index:" and "clang-tidy/..."): Index: clang-tidy/readability/CMakeLists.txt === --- clang-tidy/readabil

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added a comment. I did svn up and reuploaded the diff. I did not see any changes on files I touched, so I'm not sure it worked. http://reviews.llvm.org/D18961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53529. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/ReadabilityTidyModule.cpp docs/ReleaseNotes.rst

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added a comment. The patch doesn't apply cleanly. Please rebase it against current HEAD. http://reviews.llvm.org/D18961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added a comment. Thank you for addressing the comments! I'll commit the patch for you. http://reviews.llvm.org/D18961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53525. pilki marked 2 inline comments as done. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/Readability

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added inline comments. Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:37 @@ +36,3 @@ + +void DeletedDefaultCheck::check(const MatchFinder::MatchResult &Result) { + const StringRef Message = "%0 is explicitly defaulted but implicitly " alexfh wr

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added inline comments. Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:37 @@ +36,3 @@ + +void DeletedDefaultCheck::check(const MatchFinder::MatchResult &Result) { + const StringRef Message = "%0 is explicitly defaulted but implicitly " Will it

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53404. pilki marked 3 inline comments as done. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/Readability

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added a comment. Fixed all the comment but the one about merging the two matchers. http://reviews.llvm.org/D18961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

aaron.ballman added a subscriber: aaron.ballman. aaron.ballman added a comment. I share David's consideration for making this a compiler warning instead of a clang-tidy check. Perhaps it would make sense to gather some data using this check over some large code bases to see how often it triggers

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added inline comments. Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:36 @@ +35,3 @@ +} + +void DeletedDefaultCheck::check(const MatchFinder::MatchResult &Result) { alexfh wrote: > I would at least join matchers, since, I believe, it might be mo

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53381. pilki marked an inline comment as done. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/Readability

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. Looks good. A couple of comments. I'm happy to commit the patch for you once you answer the comments. Thank you for the work! Comment at: clang-tidy/readability/DeletedDefa

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53371. pilki marked an inline comment as done. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/Readability

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added inline comments. Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:35 @@ +34,3 @@ + this); + Finder->addMatcher(cxxMethodDecl(anyOf(isCopyAssignmentOperator(), + isMoveAssignmentOperator()), ---

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added inline comments. Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:28 @@ +27,3 @@ + // - actually deleted + // - not in template instantiation. + const auto isBadlyDefaulted = For decls there is `isInstantiated()`, which is defined as

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added a comment. I hope I answered all comments (sorry if I missed some, I'm not yet used to this UI). I have an open question about isInTemplateInstantiation, and added a test (since I missed templated function) Comment at: clang-tidy/readability/DeletedDefaultCheck.c

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53289. pilki marked 5 inline comments as done. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/Readability

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

Eugene.Zelenko added a subscriber: Eugene.Zelenko. Eugene.Zelenko added a comment. Please mention this check in docs/ReleaseNotes.rst (in alphabetical order). http://reviews.llvm.org/D18961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

I don't feel sufficiently strongly to insist - clang-tidy's mostly outside my wheelhouse anyway. As for how to go about it - my rough approach would be to write a small test case that calls an implicitly-deleted-but-explicitly-defaulted move op, run it, check the diagnostic text, find that in Diag

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added a comment. Thank you for addressing the comments! Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:27 @@ +26,3 @@ + const auto NotTemplate = unless(hasAncestor( + cxxRecordDecl(::clang::ast_matchers::isTemplateInstantiation(; +

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added a comment. In http://reviews.llvm.org/D18961#397163, @dblaikie wrote: > I'd consider just making this a compiler warning, perhaps? I honestly don't feel competent right now to write compiler warnings: clang-tidy has a nice, well defined interface. A compiler warning would force me

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added a comment. In http://reviews.llvm.org/D18961#397163, @dblaikie wrote: > I'd consider just making this a compiler warning, perhaps? I agree that it might be a good idea. However, it doesn't hurt to have this in clang-tidy (at least as a prototype - to figure out details and see how

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki updated this revision to Diff 53257. pilki marked 3 inline comments as done. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/DeletedDefaultCheck.cpp clang-tidy/readability/DeletedDefaultCheck.h clang-tidy/readability/Readability

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

I'd consider just making this a compiler warning, perhaps? On Mon, Apr 11, 2016 at 5:52 AM, Alex Pilkiewicz via cfe-commits < cfe-commits@lists.llvm.org> wrote: > pilki created this revision. > pilki added a reviewer: alexfh. > pilki added a subscriber: cfe-commits. > > Checks if constructors and

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki marked an inline comment as done. pilki added a comment. http://reviews.llvm.org/D18961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki added inline comments. Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:30 @@ +29,3 @@ + // We should actually use isExplicitlyDefaulted, but it does not exist. + Finder->addMatcher( + cxxConstructorDecl(isDefaultConstructor(), isExplicitlyDefaulted(),

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added inline comments. Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:82 @@ +81,3 @@ + } + if (const auto* copy_assignment = + Result.Nodes.getNodeAs("copy-assignment")) { nit: CopyAssignment http://reviews.llvm.org/D18961 _

Re: [PATCH] D18961: Add a readability-deleted-default clang-tidy check.

alexfh added inline comments. Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:26 @@ +25,3 @@ +void DeletedDefaultCheck::registerMatchers(MatchFinder *Finder) { + const auto NotTemplate = unless(hasAncestor( + cxxRecordDecl(::clang::ast_matchers::isTemplateInstant

[PATCH] D18961: Add a readability-deleted-default clang-tidy check.

pilki created this revision. pilki added a reviewer: alexfh. pilki added a subscriber: cfe-commits. Checks if constructors and assignment operators that are marked '= default' are actually deleted by the compiler. http://reviews.llvm.org/D18961 Files: clang-tidy/readability/CMakeLists.txt