Prazek added a comment.
Thank you Samuel, that it is valuable review!
I wanted to add the functionality of specifing stuff by options later.
should we revert this patch? It seems that those bugs are not so critical (at
least on llvm http://reviews.llvm.org/D21507).
Anyway, I will fix it in one
sbenza added inline comments.
Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:34
@@ +33,3 @@
+ hasDeclaration(functionDecl(hasName("push_back"))),
+ on(hasType(cxxRecordDecl(hasAnyName("std::vector", "llvm::SmallVector",
+
This revision was automatically updated to reflect the committed changes.
Closed by commit rL273275: [clang-tidy] Add modernize-use-emplace (authored by
Prazek).
Changed prior to commit:
http://reviews.llvm.org/D20964?vs=61214=61381#toc
Repository:
rL LLVM
http://reviews.llvm.org/D20964
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
Looks almost good now, a few comments. You'd better await for comments from
@alexfh or @sbenza before committing.
Comment at:
Prazek added a comment.
Changes after running http://reviews.llvm.org/D21507
http://reviews.llvm.org/D20964
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Prazek updated this revision to Diff 61214.
Prazek added a comment.
Changes after running og llvm code base. Adding llvm::SmallVector was a good
idea, because it has fired for much more cases and I have found 3 other bugs.
Now check doesn't fire when argument of constructor is bitfield or new
Prazek added a reviewer: sbenza.
Prazek added a comment.
You might be interested.
http://reviews.llvm.org/D20964
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Prazek marked 8 inline comments as done.
Prazek added a comment.
http://reviews.llvm.org/D20964
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Prazek updated the summary for this revision.
Prazek updated this revision to Diff 61143.
http://reviews.llvm.org/D20964
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseEmplaceCheck.cpp
clang-tidy/modernize/UseEmplaceCheck.h
Prazek added inline comments.
Comment at: test/clang-tidy/modernize-use-emplace.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s modernize-use-emplace %t
+
+namespace std {
hokein wrote:
> Could you also add the following case in the test?
> ```
> vector
hokein added a comment.
A few comments.
Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:59
@@ -57,2 +58,3 @@
CheckFactories.registerCheck("modernize-use-nullptr");
+CheckFactories.registerCheck("modernize-use-emplace");
Prazek added a reviewer: hokein.
Prazek added a subscriber: hokein.
Prazek added a comment.
@alexfh or @hokein ping
http://reviews.llvm.org/D20964
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
Prazek updated this revision to Diff 60662.
Prazek marked an inline comment as done.
Prazek added a comment.
Runned on LLVM and bug fixed one thing
http://reviews.llvm.org/D20964
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
vsk added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:41
@@ +40,3 @@
+ // (and destructed) as in push_back case.
+ auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+ ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr",
sbarzowski added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:41
@@ +40,3 @@
+ // (and destructed) as in push_back case.
+ auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+ ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr",
vsk added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:41
@@ +40,3 @@
+ // (and destructed) as in push_back case.
+ auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+ ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr",
vsk added a subscriber: vsk.
vsk added a comment.
@Eugene.Zelenko thanks for pointing this out, I had totally missed this patch!
Once we get this reviewed I'm willing to abandon my version. Some comments
inline --
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:26
@@
Eugene.Zelenko added a comment.
There are alternative implementation in http://reviews.llvm.org/D21185. Will be
good idea to how one which take the best from both :-)
http://reviews.llvm.org/D20964
___
cfe-commits mailing list
Prazek added a comment.
In http://reviews.llvm.org/D20964#448551, @Eugene.Zelenko wrote:
> In http://reviews.llvm.org/D20964#448525, @Prazek wrote:
>
> > In http://reviews.llvm.org/D20964#448455, @Eugene.Zelenko wrote:
> >
> > > I think will be good idea to try this check with LLVM STL too.
> >
Eugene.Zelenko added a comment.
In http://reviews.llvm.org/D20964#448525, @Prazek wrote:
> In http://reviews.llvm.org/D20964#448455, @Eugene.Zelenko wrote:
>
> > I think will be good idea to try this check with LLVM STL too.
>
>
> You mean llvm::SmallVector stuff?
No, I meant to build example
Prazek added a comment.
In http://reviews.llvm.org/D20964#448455, @Eugene.Zelenko wrote:
> I think will be good idea to try this check with LLVM STL too.
You mean llvm::SmallVector stuff?
http://reviews.llvm.org/D20964
___
cfe-commits mailing
Prazek updated this revision to Diff 59580.
Prazek marked 4 inline comments as done.
Prazek added a comment.
- Review fixes
http://reviews.llvm.org/D20964
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseEmplaceCheck.cpp
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
I think will be good idea to try this check with LLVM STL too.
Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:47
@@ +46,3 @@
+
+In this case the calls of push_back won't be
sbarzowski added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:40
@@ +39,3 @@
+ // passed pointer because smart pointer won't be constructed
+ // (and destructed) as in push_back case.
+ auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
Prazek marked 2 inline comments as done.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:40
@@ +39,3 @@
+ // passed pointer because smart pointer won't be constructed
+ // (and destructed) as in push_back case.
+ auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
sbarzowski added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:40
@@ +39,3 @@
+ // passed pointer because smart pointer won't be constructed
+ // (and destructed) as in push_back case.
+ auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
Prazek updated this revision to Diff 59568.
Prazek added a comment.
- Post review fix
http://reviews.llvm.org/D20964
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseEmplaceCheck.cpp
clang-tidy/modernize/UseEmplaceCheck.h
Prazek added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:31
@@ +30,3 @@
+on(hasType(cxxRecordDecl(hasName(VectorName)
+ .bind("call");
+
ok, std::list works for me. I just don't want to spend much
sbarzowski added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:30
@@ +29,3 @@
+ cxxMemberCallExpr(hasDeclaration(functionDecl(hasName(PushBackName))),
+on(hasType(cxxRecordDecl(hasName(VectorName)
+ .bind("call");
dblaikie added a subscriber: dblaikie.
dblaikie added a comment.
Could you describe in more detail (ideally in the original patch review
summary/description) what this transformation does? Under what conditions does
it suggest emplace, etc.
http://reviews.llvm.org/D20964
Prazek marked 2 inline comments as done.
Prazek added a comment.
http://reviews.llvm.org/D20964
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Prazek marked 4 inline comments as done.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:29
@@ +28,3 @@
+ auto callPushBack =
+ cxxMemberCallExpr(hasDeclaration(functionDecl(hasName(PushBackName))),
+
Prazek updated this revision to Diff 59562.
Prazek added a comment.
Learning how to use arc
http://reviews.llvm.org/D20964
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseEmplaceCheck.cpp
Prazek updated this revision to Diff 59561.
Prazek marked an inline comment as done.
Prazek added a comment.
Fixed stuff
http://reviews.llvm.org/D20964
Files:
clang-tidy/modernize/UseEmplaceCheck.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/modernize-use-emplace.rst
Index:
Prazek marked 4 inline comments as done.
Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:6
@@ +5,3 @@
+
+This check would look for cases when inserting new element into an STL
+container, but the element is constructed temporarily or is constructed just
sbarzowski added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:28
@@ +27,3 @@
+
+ auto callPushBack =
+ cxxMemberCallExpr(hasDeclaration(functionDecl(hasName(PushBackName))),
Shouldn't it start with an uppercase letter?
sbarzowski added inline comments.
Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:57
@@ +56,3 @@
+
+ auto functionNameSourceRange = CharSourceRange::getCharRange(
+ PushBackExpr->getExprLoc(), Call->getArg(0)->getExprLoc());
capital letter?
Prazek updated this revision to Diff 59558.
Prazek added a comment.
- Fix
http://reviews.llvm.org/D20964
Files:
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tidy/modernize/UseEmplaceCheck.cpp
clang-tidy/modernize/UseEmplaceCheck.h
38 matches
Mail list logo
