[PATCH] D18961: Add a readability-deleted-default clang-tidy check.
Alex Pilkiewicz via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 12 04:42:47 PDT 2016
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 more effective that way.
>
> cxxMethodDecl(anyOf(cxxConstructorDecl().bind("constructor"),
> isCopyAssignmentOperator(),
> isMoveAssignmentOperator()),
> isBadlyDefaulted)
> .bind("assignment")
>
> Optionally, you might want to inline the `isBadlyDefaulted` matcher (without the external `allOf` matcher).
>
> As for restructuring the `check()` method, I don't insist.
>
I'm a bit confused by this suggestion. It will bind to "assignment" even when it is a constructor. It works because we get "constructor" first in check(), but I find the resulting contract between the check() and registerMatchers awkward.
================
Comment at: clang-tidy/readability/DeletedDefaultCheck.cpp:49
@@ +48,3 @@
+ "constructor";
+ return;
+ }
----------------
alexfh wrote:
> If you don't feel strongly, I'd mildly suggest to turn `if { ... return }` to a chain of `if/else` (also the top-level ones). The code will become denser, but it will be easier to see the whole logic in a glance.
I thought that in the clang codebase, return were preferred to 'else'. Done.
http://reviews.llvm.org/D18961
More information about the cfe-commits
mailing list