alexfh added a comment.
Missed a couple of comments. Anyway, I'm fixing these myself as a part of
commit.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:42
@@ +41,3 @@
+auto MoveRange = CharSourceRange::getCharRange(CallMove->getSourceRange());
+auto FileMove
This revision was automatically updated to reflect the committed changes.
Closed by commit rL254070: [clang-tidy] Const std::move() argument ClangTidy
check (authored by alexfh).
Changed prior to commit:
http://reviews.llvm.org/D12031?vs=41061&id=41146#toc
Repository:
rL LLVM
http://reviews
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
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::makeF
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/MoveConstantArgument
alexfh added a comment.
Thank you for the update! Looks almost right. A few minor comments.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:34
@@ +33,3 @@
+ const Expr *Arg = CallMove->getArg(0);
+ auto *Context = Result.Context;
+
Looks like you're
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 "
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::makeFileCharRang
alexfh added inline comments.
Comment at: clang-tidy/misc/MoveConstantArgumentCheck.cpp:23
@@ +22,3 @@
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) {
The problem is that each template class or function can have several
repre
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
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->a
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/CMakeList
alexfh added inline comments.
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:
> Plea
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 a
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.c
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
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/mi
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 @@
+
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
sbenza added inline comments.
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);
You can move
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_clan
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
22 matches
Mail list logo
