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:
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
pilki added inline comments.
Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:35
@@ +34,3 @@
+ this);
+ Finder->addMatcher(cxxMethodDecl(anyOf(isCopyAssignmentOperator(),
+ isMoveAssignmentOperator()),
---
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
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
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
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
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
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(;
+
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
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
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
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
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
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(),
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
_
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
34 matches
Mail list logo
