Thank you for the check!
Looks generally fine, but needs some polishing to match LLVM style.
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:14
@@ +13,3 @@
+
+// #include "clang/AST/ASTContext.h"
+// #include "clang/AST/Decl.h"
----------------
Please remove commented lines.
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:39
@@ +38,3 @@
+void UniqueptrResetRelease::check(
+ const ast_matchers::MatchFinder::MatchResult &result) {
+ const auto* reset_member =
result.Nodes.getNodeAs<MemberExpr>("reset_member");
----------------
You're already using ast_matchers, so you can avoid it here.
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:40
@@ +39,3 @@
+ const ast_matchers::MatchFinder::MatchResult &result) {
+ const auto* reset_member =
result.Nodes.getNodeAs<MemberExpr>("reset_member");
+ const auto* release_member =
----------------
Please use CamelCase for local variables.
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:55
@@ +54,3 @@
+
+ if (reset_member->isArrow()) {
+ left_text = "*" + left_text;
----------------
In LLVM style single-statement if/for/... bodies don't need braces (and should
not be on the same line with the if/for/...).
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:68
@@ +67,3 @@
+ diag(reset_member->getExprLoc(),
+ "Prefer std::move over ptr1.reset(ptr2.release())")
+ << FixItHint::CreateReplacement(
----------------
Clang warnings don't start with a capital letter. We try to move to this style
in clang-tidy as well.
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:68
@@ +67,3 @@
+ diag(reset_member->getExprLoc(),
+ "Prefer std::move over ptr1.reset(ptr2.release())")
+ << FixItHint::CreateReplacement(
----------------
alexfh wrote:
> Clang warnings don't start with a capital letter. We try to move to this
> style in clang-tidy as well.
Maybe "prefer ptr1 = std::move(ptr2); ..."? It would be more wordy, but also
clearer, I think.
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:74
@@ +73,3 @@
+
+} // namespace tidy
+} // namespace clang
----------------
LLVM style: one space before comments, please.
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.h:28
@@ +27,3 @@
+public:
+ using ClangTidyCheck::ClangTidyCheck;
+ void registerMatchers(ast_matchers::MatchFinder *finder) override;
----------------
Unfortunately, delegated constructors are not implemented in some of the
compilers supported by Clang (VS2012). You need to manually delegate the
constructor.
================
Comment at: clang-tidy/misc/UniqueptrResetRelease.h:29
@@ +28,3 @@
+ using ClangTidyCheck::ClangTidyCheck;
+ void registerMatchers(ast_matchers::MatchFinder *finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &result) override;
----------------
Here and below: in LLVM style names of local variables and function parameters
should start with a capital letter.
================
Comment at: test/clang-tidy/uniqueptr-reset-release.cpp:4
@@ +3,3 @@
+
+// CHECK-NOT: warning
+
----------------
check_clang_tidy.sh calls FileCheck with -implicit-check-not='warning:|error:',
so this is superfluous.
================
Comment at: test/clang-tidy/uniqueptr-reset-release.cpp:35
@@ +34,3 @@
+ a.reset(c.release());
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Prefer std::move over
ptr1.reset(ptr2.release()) [misc-uniqueptr-reset-release]
+ // CHECK-FIXES: a = std::move(c);
----------------
You can omit the " [misc-uniqueptr-reset-release]" part in all the messages
except for the first one. It will make the test slightly more readable.
http://reviews.llvm.org/D6485
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
