Re: [PATCH] D12031: Const std::move() argument ClangTidy check
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
Awesome! Thank you for the new check!
There are a couple of nits, but I'll fix these before submitting the patch.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:42
@@ +41,3 @@
+auto MoveRange = CharSourceRange::getCharRange(CallMove->getSourceRange());
+auto FileMoveRange = Lexer::makeFileCharRange(MoveRange, SM,
getLangOpts());
+if (!FileMoveRange.isValid()) {
s/auto/CharSourceRange/
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:47
@@ +46,3 @@
+bool IsVariable = isa(Arg);
+DiagnosticBuilder DB =
+diag(FileMoveRange.getBegin(), "std::move of the %select{|const }0"
I'd replace `DiagnosticBuilder DB` with `auto Diag` - this is more a local
convention, not a general rule.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:57
@@ +56,3 @@
+ Arg->getLocStart()),
+SM, getLangOpts());
+CharSourceRange AfterArgumentsRange = Lexer::makeFileCharRange(
This can probably happen when `Arg` comes from another macro (or a macro
argument). A test can be added later.
http://reviews.llvm.org/D12031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym added a comment. Thanks alexfh! I've addressed your comments and uploaded new patch. PTAL Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:56 @@ +55,3 @@ +<< IsConstArg << IsVariable << IsTriviallyCopyable +<< FixItHint::CreateRemoval(Lexer::makeFileCharRange( + CharSourceRange::getCharRange(CallMove->getLocStart(), alexfh wrote: > After some thinking, there may be cases where the range of the whole > expression can be translated to a file char range, but a sub-range of it > can't. So you need to check the validity of the results of both > `makeFileCharRange` calls in this expression before creating a fixit hint > with the resulting ranges. > > Also, it seems reasonable to issue a warning in any case, and fix-it hints > only when we are rather certain that we can apply them safely (which the > validity of all `makeFileCharRange` results should tell us about). I've changed: now BeforeArgumentsRange and AfterArgumentsRange are calculated if they are valid then Removal is created. But probably it's better to write a test when some of these ranges are valid, could you please advice when it can be? http://reviews.llvm.org/D12031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym updated this revision to Diff 41061.
dvadym marked 8 inline comments as done.
dvadym added a comment.
In this patch alexfh comments addressed
http://reviews.llvm.org/D12031
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/MoveConstantArgumentCheck.cpp
clang-tidy/misc/MoveConstantArgumentCheck.h
test/clang-tidy/move-const-arg.cpp
Index: test/clang-tidy/move-const-arg.cpp
===
--- test/clang-tidy/move-const-arg.cpp
+++ test/clang-tidy/move-const-arg.cpp
@@ -0,0 +1,73 @@
+// RUN: %check_clang_tidy %s misc-move-const-arg %t -- -- -std=c++11
+
+namespace std {
+template struct remove_reference;
+
+template struct remove_reference { typedef _Tp type; };
+
+template struct remove_reference<_Tp &> { typedef _Tp type; };
+
+template struct remove_reference<_Tp &&> { typedef _Tp type; };
+
+template
+constexpr typename std::remove_reference<_Tp>::type &(_Tp &&__t);
+
+} // namespace std
+
+class A {
+public:
+ A() {}
+ A(const A ) {}
+ A(A &) {}
+};
+
+int f1() {
+ return std::move(42);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of trivially-copyable type has no effect; remove std::move() [misc-move-const-arg]
+ // CHECK-FIXES: return 42;
+}
+
+int f2(int x2) {
+ return std::move(x2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of trivially-copyable type
+ // CHECK-FIXES: return x2;
+}
+
+int *f3(int *x3) {
+ return std::move(x3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of trivially-copyable type
+ // CHECK-FIXES: return x3;
+}
+
+A f4(A x4) { return std::move(x4); }
+
+A f5(const A x5) {
+ return std::move(x5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable
+ // CHECK-FIXES: return x5;
+}
+
+template T f6(const T x6) { return std::move(x6); }
+
+void f7() { int a = f6(10); }
+
+#define M1(x) x
+void f8() {
+ const A a;
+ M1(A b = std::move(a);)
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: std::move of the const variable
+ // CHECK-FIXES: M1(A b = a;)
+}
+
+#define M2(x) std::move(x)
+int f9() { return M2(1); }
+
+template T f10(const int x10) {
+ return std::move(x10);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable
+ // CHECK-FIXES: return x10;
+}
+void f11() {
+ f10(1);
+ f10(1);
+}
Index: clang-tidy/misc/MoveConstantArgumentCheck.h
===
--- clang-tidy/misc/MoveConstantArgumentCheck.h
+++ clang-tidy/misc/MoveConstantArgumentCheck.h
@@ -0,0 +1,31 @@
+//===--- MoveConstandArgumentCheck.h - clang-tidy -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+class MoveConstantArgumentCheck : public ClangTidyCheck {
+public:
+ MoveConstantArgumentCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
Index: clang-tidy/misc/MoveConstantArgumentCheck.cpp
===
--- clang-tidy/misc/MoveConstantArgumentCheck.cpp
+++ clang-tidy/misc/MoveConstantArgumentCheck.cpp
@@ -0,0 +1,72 @@
+//===--- MoveConstandArgumentCheck.cpp - clang-tidy ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "MoveConstantArgumentCheck.h"
+
+#include
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+using namespace ast_matchers;
+
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus)
+return;
+ Finder->addMatcher(callExpr(unless(isInTemplateInstantiation()),
+ callee(functionDecl(hasName("::std::move"
+ .bind("call-move"),
+ this);
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult ) {
+ const auto *CallMove = Result.Nodes.getNodeAs("call-move");
+ if (CallMove->getNumArgs() != 1)
+return;
+ const Expr
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym updated this revision to Diff 40751.
dvadym marked 3 inline comments as done.
dvadym added a comment.
New in this upload:
1.Adding checking that language is C++
2.Not consider calls of std::move in template instantation
3.Creating CharSourceRange for removal
4.Using Lexer::makeFileCharRange( for processing move call in macros
http://reviews.llvm.org/D12031
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/MoveConstantArgumentCheck.cpp
clang-tidy/misc/MoveConstantArgumentCheck.h
test/clang-tidy/move-const-arg.cpp
Index: test/clang-tidy/move-const-arg.cpp
===
--- test/clang-tidy/move-const-arg.cpp
+++ test/clang-tidy/move-const-arg.cpp
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s misc-move-const-arg %t -- -- -std=c++11
+
+namespace std {
+template struct remove_reference;
+
+template struct remove_reference { typedef _Tp type; };
+
+template struct remove_reference<_Tp &> { typedef _Tp type; };
+
+template struct remove_reference<_Tp &&> { typedef _Tp type; };
+
+template
+constexpr typename std::remove_reference<_Tp>::type &(_Tp &&__t);
+
+} // namespace std
+
+class A {
+public:
+ A() {}
+ A(const A ) {}
+ A(A &) {}
+};
+
+int f1() {
+ return std::move(42);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of
+ // trivially-copyable type has no effect; remove std::move().
+ // CHECK-FIXES: return 42;
+}
+
+int f2(int x2) {
+ return std::move(x2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of
+ // trivially-copyable type has no effect; remove std::move().
+ // CHECK-FIXES: return x2;
+}
+
+int *f3(int *x3) {
+ return std::move(x3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of
+ // trivially-copyable type has no effect; remove std::move().
+ // CHECK-FIXES: return x3;
+}
+
+A f4(A x4) { return std::move(x4); }
+
+A f5(const A x5) {
+ return std::move(x5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable
+ // has no effect; remove std::move().
+ // CHECK-FIXES: return x5;
+}
+
+template T f6(const T x6) { return std::move(x6); }
+
+void f7() { int a = f6(10); }
+
+#define M1(x) x
+void f8() {
+ const A a;
+ M1(A b = std::move(a);)
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: std::move of the const variable
+ // has no effect; remove std::move().
+ // CHECK-FIXES: M1(A b = a;)
+}
+
+#define M2(x) std::move(x)
+int f9() { return M2(1); }
Index: clang-tidy/misc/MoveConstantArgumentCheck.h
===
--- clang-tidy/misc/MoveConstantArgumentCheck.h
+++ clang-tidy/misc/MoveConstantArgumentCheck.h
@@ -0,0 +1,31 @@
+//===--- MoveConstandArgumentCheck.h - clang-tidy -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+class MoveConstantArgumentCheck : public ClangTidyCheck {
+public:
+ MoveConstantArgumentCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
Index: clang-tidy/misc/MoveConstantArgumentCheck.cpp
===
--- clang-tidy/misc/MoveConstantArgumentCheck.cpp
+++ clang-tidy/misc/MoveConstantArgumentCheck.cpp
@@ -0,0 +1,70 @@
+//===--- MoveConstandArgumentCheck.cpp - clang-tidy ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "MoveConstantArgumentCheck.h"
+
+#include
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+using namespace ast_matchers;
+
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder *Finder) {
+ if (!getLangOpts().CPlusPlus)
+return;
+ Finder->addMatcher(callExpr(unless(isInTemplateInstantiation()),
+ callee(functionDecl(hasName("::std::move"
+ .bind("call-move"),
+ this);
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult ) {
+
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym added a comment.
Thanks for comments! PTAL
Since it's added checking of trivially copyable arguments of move, it's needed
to rename this check, what do you think about MoveNoEffectCheck?
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:11
@@ +10,3 @@
+#include "MoveConstantArgumentCheck.h"
+
+namespace clang {
aaron.ballman wrote:
> > I didn't find how it can be done, could you please advice?
>
> This is the usual way we do it (in the registerMatchers() function):
> ```
> if (!getLangOpts().CPlusPlus)
> return;
> ```
>
Thanks
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:23
@@ +22,3 @@
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult ) {
alexfh wrote:
> The problem is that each template class or function can have several
> representations in the AST: one for the template definition and one for each
> instantiation. Usually, we don't need to even look at the instantiations,
> when we want to reason about the code in the general case. You can filter out
> expressions belonging to template instantiations using this narrowing
> matcher: `unless(isInTemplateInstantiation())`. And for template definitions
> the type will be marked as dependent.
Great, thank you. It works
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:40
@@ +39,3 @@
+diag(CallMove->getLocStart(), "std::move of the %select{|const "
+ "}0%select{expression|variable}1 %select{|of
"
+ "trivially-copyable type }2has no effect; "
alexfh wrote:
> dvadym wrote:
> > Could you please advice how can I correctly make removal?
> > I expected that
> > FixItHint::CreateRemoval(SourceRange(CallMove->getLocStart(),
> > Arg->getLocStart())) removes "std::move(" but it removes
> > "std::move(varname", so from a "move" call only ")" is left
> `FixItHint::CreateRemoval` and many other methods accept `CharSourceRange`
> instead of `SourceRange`. The former is a `SourceRange` + a flag telling
> whether the range should be treated as a character range or a token range. By
> default, `SourceRange` is converted to a `CharSourceRange` marked as a token
> range. So your current code creates a `FixItHint` that removes tokens from
> `std` to `varname` inclusive. If you want to delete everything from `std` to
> just before `varname`, you can create a character range from
> `CallMove->getLocStart()` to `Arg->getLocStart().getLocWithOffset(-1)`.
>
> However, when there's something between `std::move(` and `varname`
> (whitespace and/or comment(s)), might want to delete just `std::move(`. In
> this case you can take `CallMove->getCallee()' (which will correspond to
> `std::move`), and then find the first '(' token after it's end location. It's
> probably a rare case though, so let's go for the simpler solution for now
> (with `getLocWithOffset` and character ranges).
Thanks, creating CharSourceRange makes the trick. Also as we talked offline
I've added a call of Lexer::makeFileCharRange( for processing move call in
macros. Please have a look.
http://reviews.llvm.org/D12031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
aaron.ballman added inline comments.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:11
@@ +10,3 @@
+#include "MoveConstantArgumentCheck.h"
+
+namespace clang {
> I didn't find how it can be done, could you please advice?
This is the usual way we do it (in the registerMatchers() function):
```
if (!getLangOpts().CPlusPlus)
return;
```
Comment at: test/clang-tidy/move-const-arg.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s misc-move-const-arg %t -- -- -std=c++11
+
Please run clang-format over the test files as well.
http://reviews.llvm.org/D12031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym updated this revision to Diff 39717.
dvadym marked 5 inline comments as done.
dvadym added a comment.
Addressed reviewers' comments
1.Change message generation on using diag() string substitution
2.Added copyright
3.Rebase
http://reviews.llvm.org/D12031
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/MoveConstantArgumentCheck.cpp
clang-tidy/misc/MoveConstantArgumentCheck.h
test/clang-tidy/move-const-arg.cpp
Index: test/clang-tidy/move-const-arg.cpp
===
--- test/clang-tidy/move-const-arg.cpp
+++ test/clang-tidy/move-const-arg.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy %s misc-move-const-arg %t -- -- -std=c++11
+
+namespace std {
+template
+struct remove_reference;
+
+template
+ struct remove_reference
+ { typedef _Tp type; };
+
+template
+ struct remove_reference<_Tp&>
+ { typedef _Tp type; };
+
+template
+ struct remove_reference<_Tp&&>
+ { typedef _Tp type; };
+
+template
+ constexpr typename std::remove_reference<_Tp>::type&&
+ move(_Tp&& __t);
+
+} // namespace std
+
+class A
+{
+ public:
+ A() {}
+ A(const A& rhs) {}
+ A(A&& rhs) {}
+};
+
+int f1()
+{
+ return std::move(42);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return 42;
+}
+
+int f2(int x2)
+{
+ return std::move(x2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return x2;
+}
+
+int* f3(int* x3)
+{
+ return std::move(x3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return x3;
+}
+
+A f4(A x4)
+{
+ return std::move(x4);
+}
+
+A f5(const A x5)
+{
+ return std::move(x5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable doesn't have effect. Remove std::move() or make the variable non-const.
+ // CHECK-FIXES: return x5;
+}
+
+template
+T f6(const T a)
+{
+ return std::move(a);
+}
+
+void f7()
+{
+ int a = f6(10);
+}
+
+void f8()
+{
+ A a = f6(A());
+}
+
Index: clang-tidy/misc/MoveConstantArgumentCheck.h
===
--- clang-tidy/misc/MoveConstantArgumentCheck.h
+++ clang-tidy/misc/MoveConstantArgumentCheck.h
@@ -0,0 +1,31 @@
+//===--- MoveConstandArgumentCheck.h - clang-tidy -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+class MoveConstantArgumentCheck : public ClangTidyCheck {
+public:
+ MoveConstantArgumentCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
Index: clang-tidy/misc/MoveConstantArgumentCheck.cpp
===
--- clang-tidy/misc/MoveConstantArgumentCheck.cpp
+++ clang-tidy/misc/MoveConstantArgumentCheck.cpp
@@ -0,0 +1,52 @@
+//===--- MoveConstandArgumentCheck.cpp - clang-tidy ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "MoveConstantArgumentCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+using namespace ast_matchers;
+
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("::std::move".bind("call-move"),
+ this);
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult ) {
+ const auto *CallMove = Result.Nodes.getNodeAs("call-move");
+ if (CallMove->getNumArgs() != 1)
+return;
+ const Expr *Arg = CallMove->getArg(0);
+ auto *Context = Result.Context;
+
+ bool IsTypeDependOnTemplateParameter = Arg->getType()->isDependentType();
+ if (IsTypeDependOnTemplateParameter)
+return;
+ bool IsConstArg = Arg->getType().isConstQualified();
+ bool IsTriviallyCopyable =
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym added a comment.
Thanks for review comments! Could you please have an another look and help me
with my questions?
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:10
@@ +9,3 @@
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder* Finder) {
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("::std::move".bind("call-move"),
aaron.ballman wrote:
> Please only register this matcher for C++ code.
I didn't find how it can be done, could you please advice?
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:22
@@ +21,3 @@
+ bool IsTypeDependOnTemplateParameter =
+ false; // my first guess was type->getTypeClass () == 30 but it doesn't
+ // work in some cases. Could you please advice better solution.
alexfh wrote:
> aaron.ballman wrote:
> > Arg->getType()->isDependentType() should do what you want, if I understand
> > you properly.
> Yep, should be what you need.
It doesn't do what it's needed. See for example function f6, f7, f8 in tests.
::check method is called once on any instantion of f6,
Arg->getType()->isDependentType() returns false, so the check returns 2
warning.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:29
@@ +28,3 @@
+ if (IsConstArg || IsTriviallyCopyable) {
+bool IsVariable = dyn_cast(Arg) != nullptr;
+std::string message = "std::move of the ";
aaron.ballman wrote:
> isa instead of dyn_cast and check against nullptr.
Thanks
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:30
@@ +29,3 @@
+bool IsVariable = dyn_cast(Arg) != nullptr;
+std::string message = "std::move of the ";
+message += IsConstArg ? "const " : "";
alexfh wrote:
> alexfh wrote:
> > Please don't string += as a way to build messages. This creates a temporary
> > each time and reallocates the string buffer. Use one of the these:
> >
> > std::string message = (llvm::Twine("...") + "..." + "...").str()
> >
> > (only in a single expression, i.e. don't create variables of the
> > llvm::Twine type, as this can lead to dangling string references), or:
> >
> > std::string buffer;
> > llvm::raw_string_ostream message(buffer);
> > message << "...";
> > message << "...";
> > // then use message.str() where you would use an std::string.
> >
> > The second alternative would be more suitable for this kind of code.
> >
> > BUT, even this is not needed in the specific case of producing diagnostics,
> > as clang provides a powerful template substitution mechanism, and your code
> > could be written more effectively:
> >
> > diag("std::move of the %select{const |}0 %select{variable|expression}1
> > ...") << IsConstArg << IsVariable << ...;
> This should read:
> "Please don't use string += as a way to build messages. This creates a
> temporary each time and reallocates the string buffer. Instead, use one of
> these patterns:"
Thanks, it made code much clearer
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:40
@@ +39,3 @@
+diag(CallMove->getLocStart(), "std::move of the %select{|const "
+ "}0%select{expression|variable}1 %select{|of
"
+ "trivially-copyable type }2has no effect; "
Could you please advice how can I correctly make removal?
I expected that
FixItHint::CreateRemoval(SourceRange(CallMove->getLocStart(),
Arg->getLocStart())) removes "std::move(" but it removes "std::move(varname",
so from a "move" call only ")" is left
http://reviews.llvm.org/D12031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym added a comment.
Thanks alexfh and sbenza for comments!
I've addressed most of them. Could you please advice how to find that some
expression has type which is template dependent?
Regards,
Vadym
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:20
@@ +19,3 @@
+ const auto* CallMove = result.Nodes.getNodeAs("call-move");
+ if (CallMove->getNumArgs() != 1) return;
+ const Expr* Arg = CallMove->getArg(0);
sbenza wrote:
> You can move both checks to the matcher.
> Something like:
>
> callExpr(callee(functionDecl(hasName("::std::move"))),
> argumentCountIs(1),
> hasArgument(0, expr(hasType(isConstQualified()
Thanks, according to alexfh comments I've decided to apply this check not only
for const but also for trivially copyable types.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:31
@@ +30,3 @@
+std::string ArgString(sm->getCharacterData(ArgBegin), length);
+diag(CallMove->getLocStart(), "move of const variable")
+<< FixItHint::CreateReplacement(MoveRange, ArgString);
alexfh wrote:
> The message could be more helpful. For example, "std::move of the const
> variable '%0' doesn't have effect". We could also add a recommendation on how
> to fix that (e.g. "remove std::move() or make the variable non-const").
>
> Also, in case it's not a variable (DeclRefExpr), the warning shouldn't say
> "variable".
Thanks, I've addressed your comments.
Comment at: test/clang-tidy/move-const-arg.cpp:29
@@ +28,3 @@
+{
+ return std::move(42);
+}
alexfh wrote:
> That also doesn't look like a reasonable use of std::move. We should probably
> extend this check to flag std::move applied to rvalues (in general, not only
> usages of const variables), scalar types, pointer types and probably also all
> other trivially-copyable types (I don't immediately see why moving those
> could ever be better than copying). These warnings shouldn't trigger for
> dependent types and in template instantiations.
Thanks, I've implemented for trivially copyable types. But I didn't find how to
find dependent types: Arg->isTypeDependent(), Arg->isInstantiationDependent(),
Arg->getType()->isDependentType() doesn't look like solving this problem. Could
you please advice?
Comment at: test/clang-tidy/move-const-arg.cpp:41
@@ +40,3 @@
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: move of const variable
[move-const-arg]
+ // CHECK-FIXES: return x;
+}
alexfh wrote:
> Please make the checked code lines unique to avoid matching in a wrong place.
> FileCheck (the utility that is called by the check_clang_tidy.py script to
> verify the `// CHECK-...` lines,
> http://llvm.org/docs/CommandGuide/FileCheck.html) doesn't take the location
> of the `// CHECK-FIXES:` line in the test file into consideration, it just
> verifies that the fixed file has a subsequence of lines that matches all `//
> CHECK-FIXES` lines in that order. Here, for example, `return x;` would
> equally match, if the check would fix line 34 instead of line 39. We could
> solve this by numbering lines so that CHECK-FIXES patterns could refer to the
> line numbers, but until we do that (if we decide so), making the checked
> lines unique is the way to go (e.g. by using different variable names in each
> test case instead of just `x`).
Thanks, I've set different names to variables
http://reviews.llvm.org/D12031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym updated the summary for this revision.
dvadym updated this revision to Diff 39077.
dvadym marked 6 inline comments as done.
dvadym added a comment.
1.Most comments addressed
2.Taking into consideration applying move to trivially copyable objects
3.Different message if move argument variable or expression.
It's not addressed yet case when move argument is depend in some way on
template argument. There is comment in code about this.
http://reviews.llvm.org/D12031
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/MoveConstantArgumentCheck.cpp
clang-tidy/misc/MoveConstantArgumentCheck.h
test/clang-tidy/move-const-arg.cpp
Index: test/clang-tidy/move-const-arg.cpp
===
--- test/clang-tidy/move-const-arg.cpp
+++ test/clang-tidy/move-const-arg.cpp
@@ -0,0 +1,72 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-move-const-arg %t
+// REQUIRES: shell
+
+namespace std {
+// Directly copied from the stl.
+template
+struct remove_reference;
+
+template
+ struct remove_reference
+ { typedef _Tp type; };
+
+template
+ struct remove_reference<_Tp&>
+ { typedef _Tp type; };
+
+template
+ struct remove_reference<_Tp&&>
+ { typedef _Tp type; };
+
+template
+ constexpr typename std::remove_reference<_Tp>::type&&
+ move(_Tp&& __t);
+
+} // namespace std
+
+class A
+{
+ public:
+ A() {}
+ A(const A& rhs) {}
+ A(A&& rhs) {}
+};
+
+int f1()
+{
+ return std::move(42);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return 42;
+}
+
+int f2(int x2)
+{
+ return std::move(x2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return x2;
+}
+
+int* f3(int* x3)
+{
+ return std::move(x3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return x3;
+}
+
+A f4(A x4)
+{
+ return std::move(x4);
+}
+
+A f5(const A x5)
+{
+ return std::move(x5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable doesn't have effect. Remove std::move() or make the variable non-const.
+ // CHECK-FIXES: return x5;
+}
+
+template
+T f6(const T a)
+{
+ return std::move(a);
+}
Index: clang-tidy/misc/MoveConstantArgumentCheck.h
===
--- clang-tidy/misc/MoveConstantArgumentCheck.h
+++ clang-tidy/misc/MoveConstantArgumentCheck.h
@@ -0,0 +1,17 @@
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+class MoveConstantArgumentCheck : public ClangTidyCheck {
+public:
+ MoveConstantArgumentCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/misc/MoveConstantArgumentCheck.cpp
===
--- clang-tidy/misc/MoveConstantArgumentCheck.cpp
+++ clang-tidy/misc/MoveConstantArgumentCheck.cpp
@@ -0,0 +1,51 @@
+#include "MoveConstantArgumentCheck.h"
+
+#include
+using namespace std;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+using namespace ast_matchers;
+
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder* Finder) {
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("::std::move".bind("call-move"),
+ this);
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult& Result) {
+ const auto* CallMove = Result.Nodes.getNodeAs("call-move");
+ if (CallMove->getNumArgs() != 1) return;
+ const Expr* Arg = CallMove->getArg(0);
+ auto* Context = Result.Context;
+
+ bool IsTypeDependOnTemplateParameter =
+ false; // my first guess was type->getTypeClass () == 30 but it doesn't
+ // work in some cases. Could you please advice better solution.
+ if (IsTypeDependOnTemplateParameter) return;
+ bool IsConstArg = Arg->getType().isConstQualified();
+ bool IsTriviallyCopyable = Arg->getType().isTriviallyCopyableType(*Context);
+
+ if (IsConstArg || IsTriviallyCopyable) {
+bool IsVariable = (dyn_cast(Arg) != nullptr);
+string message = "std::move of the ";
+message += IsConstArg ? "const " : "";
+message += IsVariable ? "variable " : "expression ";
+message += IsTriviallyCopyable ? "of trivially-copyable type " : "";
+message += "doesn't have effect. ";
+message += "Remove std::move()";
+message += IsConstArg && IsVariable ? " or make the variable non-const." : ".";
+
+SourceRange RemoveRange1(CallMove->getLocStart(), Arg->getLocStart());
+
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
dvadym updated this revision to Diff 39080.
dvadym marked an inline comment as done.
dvadym added a comment.
Small clean-up
http://reviews.llvm.org/D12031
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/MoveConstantArgumentCheck.cpp
clang-tidy/misc/MoveConstantArgumentCheck.h
test/clang-tidy/move-const-arg.cpp
Index: test/clang-tidy/move-const-arg.cpp
===
--- test/clang-tidy/move-const-arg.cpp
+++ test/clang-tidy/move-const-arg.cpp
@@ -0,0 +1,71 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-move-const-arg %t
+
+namespace std {
+// Directly copied from the stl.
+template
+struct remove_reference;
+
+template
+ struct remove_reference
+ { typedef _Tp type; };
+
+template
+ struct remove_reference<_Tp&>
+ { typedef _Tp type; };
+
+template
+ struct remove_reference<_Tp&&>
+ { typedef _Tp type; };
+
+template
+ constexpr typename std::remove_reference<_Tp>::type&&
+ move(_Tp&& __t);
+
+} // namespace std
+
+class A
+{
+ public:
+ A() {}
+ A(const A& rhs) {}
+ A(A&& rhs) {}
+};
+
+int f1()
+{
+ return std::move(42);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return 42;
+}
+
+int f2(int x2)
+{
+ return std::move(x2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return x2;
+}
+
+int* f3(int* x3)
+{
+ return std::move(x3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of trivially-copyable type doesn't have effect. Remove std::move().
+ // CHECK-FIXES: return x3;
+}
+
+A f4(A x4)
+{
+ return std::move(x4);
+}
+
+A f5(const A x5)
+{
+ return std::move(x5);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable doesn't have effect. Remove std::move() or make the variable non-const.
+ // CHECK-FIXES: return x5;
+}
+
+template
+T f6(const T a)
+{
+ return std::move(a);
+}
Index: clang-tidy/misc/MoveConstantArgumentCheck.h
===
--- clang-tidy/misc/MoveConstantArgumentCheck.h
+++ clang-tidy/misc/MoveConstantArgumentCheck.h
@@ -0,0 +1,17 @@
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+class MoveConstantArgumentCheck : public ClangTidyCheck {
+public:
+ MoveConstantArgumentCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/misc/MoveConstantArgumentCheck.cpp
===
--- clang-tidy/misc/MoveConstantArgumentCheck.cpp
+++ clang-tidy/misc/MoveConstantArgumentCheck.cpp
@@ -0,0 +1,49 @@
+#include "MoveConstantArgumentCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+using namespace ast_matchers;
+
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder* Finder) {
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("::std::move".bind("call-move"),
+ this);
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult& Result) {
+ const auto* CallMove = Result.Nodes.getNodeAs("call-move");
+ if (CallMove->getNumArgs() != 1) return;
+ const Expr* Arg = CallMove->getArg(0);
+ auto* Context = Result.Context;
+
+ bool IsTypeDependOnTemplateParameter =
+ false; // my first guess was type->getTypeClass () == 30 but it doesn't
+ // work in some cases. Could you please advice better solution.
+ if (IsTypeDependOnTemplateParameter) return;
+ bool IsConstArg = Arg->getType().isConstQualified();
+ bool IsTriviallyCopyable = Arg->getType().isTriviallyCopyableType(*Context);
+
+ if (IsConstArg || IsTriviallyCopyable) {
+bool IsVariable = dyn_cast(Arg) != nullptr;
+std::string message = "std::move of the ";
+message += IsConstArg ? "const " : "";
+message += IsVariable ? "variable " : "expression ";
+message += IsTriviallyCopyable ? "of trivially-copyable type " : "";
+message += "doesn't have effect. ";
+message += "Remove std::move()";
+message +=
+IsConstArg && IsVariable ? " or make the variable non-const." : ".";
+
+SourceRange RemoveRange1(CallMove->getLocStart(), Arg->getLocStart());
+SourceRange RemoveRange2(CallMove->getLocEnd(), CallMove->getLocEnd());
+diag(CallMove->getLocStart(), message)
+<< FixItHint::CreateRemoval(RemoveRange1)
+<< FixItHint::CreateRemoval(RemoveRange2);
+ }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/misc/MiscTidyModule.cpp
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
aaron.ballman added a subscriber: aaron.ballman.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:1
@@ +1,2 @@
+#include "MoveConstantArgumentCheck.h"
+
Missing new file legal text.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:9
@@ +8,3 @@
+
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder* Finder) {
+ Finder->addMatcher(
Formatting -- the * should go with Finder. May want to run clang-format over
the entire patch; there's a lot of other formatting issues I will refrain from
commenting on.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:10
@@ +9,3 @@
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder* Finder) {
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("::std::move".bind("call-move"),
Please only register this matcher for C++ code.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:22
@@ +21,3 @@
+ bool IsTypeDependOnTemplateParameter =
+ false; // my first guess was type->getTypeClass () == 30 but it doesn't
+ // work in some cases. Could you please advice better solution.
Arg->getType()->isDependentType() should do what you want, if I understand you
properly.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:29
@@ +28,3 @@
+ if (IsConstArg || IsTriviallyCopyable) {
+bool IsVariable = dyn_cast(Arg) != nullptr;
+std::string message = "std::move of the ";
isa instead of dyn_cast and check against nullptr.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:34
@@ +33,3 @@
+message += IsTriviallyCopyable ? "of trivially-copyable type " : "";
+message += "doesn't have effect. ";
+message += "Remove std::move()";
This line reads a bit strangely to me. Perhaps "has no effect" instead? Also,
our diagnostics are not full sentences, so you should remove the period, and
instead do: "; remove std::move()"
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.h:1
@@ +1,2 @@
+#include "../ClangTidy.h"
+
Missing header include guards and header legal text.
http://reviews.llvm.org/D12031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
alexfh added inline comments.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:22
@@ +21,3 @@
+ bool IsTypeDependOnTemplateParameter =
+ false; // my first guess was type->getTypeClass () == 30 but it doesn't
+ // work in some cases. Could you please advice better solution.
aaron.ballman wrote:
> Arg->getType()->isDependentType() should do what you want, if I understand
> you properly.
Yep, should be what you need.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:30
@@ +29,3 @@
+bool IsVariable = dyn_cast(Arg) != nullptr;
+std::string message = "std::move of the ";
+message += IsConstArg ? "const " : "";
Please don't string += as a way to build messages. This creates a temporary
each time and reallocates the string buffer. Use one of the these:
std::string message = (llvm::Twine("...") + "..." + "...").str()
(only in a single expression, i.e. don't create variables of the llvm::Twine
type, as this can lead to dangling string references), or:
std::string buffer;
llvm::raw_string_ostream message(buffer);
message << "...";
message << "...";
// then use message.str() where you would use an std::string.
The second alternative would be more suitable for this kind of code.
BUT, even this is not needed in the specific case of producing diagnostics, as
clang provides a powerful template substitution mechanism, and your code could
be written more effectively:
diag("std::move of the %select{const |}0 %select{variable|expression}1 ...")
<< IsConstArg << IsVariable << ...;
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:39
@@ +38,3 @@
+
+SourceRange RemoveRange1(CallMove->getLocStart(), Arg->getLocStart());
+SourceRange RemoveRange2(CallMove->getLocEnd(), CallMove->getLocEnd());
The variable names don't add much value here. Either remove the variables and
use the expressions below or give the variables more useful names. Also note
that `SourceRange` can be constructed from a single token
(`SourceRange(CallMove->getLocEnd())`).
Comment at: test/clang-tidy/move-const-arg.cpp:1
@@ +1,2 @@
+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-move-const-arg %t
+
This has changed once more, now `%check_clang_tidy` should be used instead of
the script itself. Please also rebase the patch on top of current HEAD.
Comment at: test/clang-tidy/move-const-arg.cpp:4
@@ +3,3 @@
+namespace std {
+// Directly copied from the stl.
+template
The comment is not useful here. This is a mock implementation, and it's not
important where does this come from. Please also clang-format the test with
`-style=file` (to pick up formatting options specific to tests, namely
unlimited columns limit).
Comment at: test/clang-tidy/move-const-arg.cpp:30
@@ +29,3 @@
+ A() {}
+ A(const A& rhs) {}
+ A(A&& rhs) {}
How did you find that `Arg->getType()->isDependentType()` doesn't work?
http://reviews.llvm.org/D12031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
alexfh added a comment. Vadym, what's the state of this patch? http://reviews.llvm.org/D12031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
alexfh added a comment. A few more comments. Comment at: test/clang-tidy/move-const-arg.cpp:1 @@ +1,2 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s move-const-arg %t +// REQUIRES: shell Please use check_clang_tidy.py instead: // RUN: %python %S/check_clang_tidy.py %s move-const-arg %t and remove `// REQUIRES: shell`, as it's not needed any more. Comment at: test/clang-tidy/move-const-arg.cpp:41 @@ +40,3 @@ + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: move of const variable [move-const-arg] + // CHECK-FIXES: return x; +} Please make the checked code lines unique to avoid matching in a wrong place. FileCheck (the utility that is called by the check_clang_tidy.py script to verify the `// CHECK-...` lines, http://llvm.org/docs/CommandGuide/FileCheck.html) doesn't take the location of the `// CHECK-FIXES:` line in the test file into consideration, it just verifies that the fixed file has a subsequence of lines that matches all `// CHECK-FIXES` lines in that order. Here, for example, `return x;` would equally match, if the check would fix line 34 instead of line 39. We could solve this by numbering lines so that CHECK-FIXES patterns could refer to the line numbers, but until we do that (if we decide so), making the checked lines unique is the way to go (e.g. by using different variable names in each test case instead of just `x`). http://reviews.llvm.org/D12031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12031: Const std::move() argument ClangTidy check
alexfh added a comment.
Thank you for tackling this!
A high-level comment: the check needs to be somewhat more general.
Const-qualified variables are just a specific case of an rvalue. The check
should warn on all usages of std::move with an rvalue argument (except in
templates with arguments of dependent types).
Additionally, I would extend the check (arguably, this should be a separate
check) to complain about use of std::move with trivially-copyable types, as it
seems that there's no reason to prefer moving for these types anyway (again,
with the exception of dependent types in templates).
Comment at: clang-tidy/misc/MiscTidyModule.cpp:70
@@ -68,2 +69,3 @@
CheckFactories.registerCheckUseOverrideCheck(misc-use-override);
+CheckFactories.registerCheckMoveConstantArgumentCheck(move-const-arg);
}
The name should start with misc-. Please also update the position of the
statement to maintain sorting.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:3
@@ +2,3 @@
+
+#includeiostream
+using namespace std;
I suppose this is not needed. The line below as well.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:18
@@ +17,3 @@
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult result) {
+ const auto* CallMove = result.Nodes.getNodeAsCallExpr(call-move);
Please follow LLVM naming convention
(http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).
s/result/Result/
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:24
@@ +23,3 @@
+ if (Arg-getType().isConstQualified()) {
+SourceManager* sm = result.SourceManager;
+SourceRange MoveRange(CallMove-getLocStart(), CallMove-getRParenLoc());
s/SourceManager* sm/SourceManager SM/
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:26
@@ +25,3 @@
+SourceRange MoveRange(CallMove-getLocStart(), CallMove-getRParenLoc());
+clang::SourceLocation ArgBegin(Arg-getLocStart()),
+ArgEnd(Arg-getLocEnd());
No need for clang:: as the code is inside the clang namespace.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:31
@@ +30,3 @@
+std::string ArgString(sm-getCharacterData(ArgBegin), length);
+diag(CallMove-getLocStart(), move of const variable)
+ FixItHint::CreateReplacement(MoveRange, ArgString);
The message could be more helpful. For example, std::move of the const
variable '%0' doesn't have effect. We could also add a recommendation on how
to fix that (e.g. remove std::move() or make the variable non-const).
Also, in case it's not a variable (DeclRefExpr), the warning shouldn't say
variable.
Comment at: test/clang-tidy/move-const-arg.cpp:29
@@ +28,3 @@
+{
+ return std::move(42);
+}
That also doesn't look like a reasonable use of std::move. We should probably
extend this check to flag std::move applied to rvalues (in general, not only
usages of const variables), scalar types, pointer types and probably also all
other trivially-copyable types (I don't immediately see why moving those could
ever be better than copying). These warnings shouldn't trigger for dependent
types and in template instantiations.
http://reviews.llvm.org/D12031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
