hokein added a comment. A few comments.
================ Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:59 @@ -57,2 +58,3 @@ CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr"); + CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace"); CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override"); ---------------- Alphabetical order. ================ Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:37 @@ +36,3 @@ + // We can't replace push_backs of smart pointer because + // if emplacement will fail (f.e. bad_alloc in vector) we will have leak of + // passed pointer because smart pointer won't be constructed ---------------- remove the `will` here? if emplacement fails, we will ... ================ Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:78 @@ +77,3 @@ + + // Range for constructor name and opening brace + auto ctorCallSourceRange = CharSourceRange::getCharRange( ---------------- missing `.` at the end of sentence. ================ Comment at: clang-tidy/modernize/UseEmplaceCheck.h:19 @@ +18,3 @@ + +/// This check look for cases when inserting new element into std::vector but +/// the element is constructed temporarily. ---------------- s/look/looks ================ Comment at: docs/clang-tidy/checks/modernize-use-emplace.rst:6 @@ +5,3 @@ + +This check look for cases when inserting new element into an STL +container, but the element is constructed temporarily. ---------------- s/look/looks ================ Comment at: test/clang-tidy/modernize-use-emplace.cpp:2 @@ +1,3 @@ +// RUN: %check_clang_tidy %s modernize-use-emplace %t + +namespace std { ---------------- Could you also add the following case in the test? ``` vector<vector<int>> v1; v1.push_back(1<<10); ``` Make sure the check won't change to `v1.emplace_back(1<<10)`. Since these two statements have completely different semantic. The first one adds `1024` to `v1` while the second one adds a vector with 1024 elements to `v1`. ================ Comment at: test/clang-tidy/modernize-use-emplace.cpp:94 @@ +93,3 @@ + v.push_back(S{1, 2}); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back {{..}} + // CHECK-FIXES: v.emplace_back(1, 2); ---------------- I think you can remove the `{{..}}`. http://reviews.llvm.org/D20964 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits