[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