Thanks for contributing the check! The code looks mostly good, but see a few
comments inline.
> There was already a checker which targeted C++11, however it did not contain
> any check, which
> standard is used during the analysis. Maybe a check should be added to
> suppress C++11 specific
> warnings when clang tidy is invoked in C++03 mode?
We could bail out in the check() method it the language standard is below
C++11. Ideally, standard-specific checks would be able to avoid registering
matchers when parsing code not in supported standard mode. However, there's no
good way to do this as we don't know which minimum standard version the code is
supposed to be compatible with. And at the point where we register matchers,
there's no information on the list of files and the used compilation options
yet. So looking at the standard in the check() method is the best we can do.
================
Comment at: clang-tidy/readability/ShrinkToFitCheck.cpp:48
@@ +47,3 @@
+ const auto CopyCtorCall = constructExpr(hasArgument(
+ 0, anyOf(memberExpr(member(valueDecl().bind("CtorParam"))),
+ declRefExpr(hasDeclaration(valueDecl().bind("CtorParam"))))));
----------------
Maybe also handle pointer-type variables/data members?
================
Comment at: clang-tidy/readability/ShrinkToFitCheck.cpp:71
@@ +70,3 @@
+ CharSourceRange::getTokenRange(ContainerToShrink->getSourceRange()),
+ *Result.SourceManager, LangOptions());
+ ReplacementText += ".shrink_to_fit()";
----------------
Please use Result.Context->getLangOpts() instead (I've fixed the other two
checks using LangOptions()).
================
Comment at: clang-tidy/readability/ShrinkToFitCheck.cpp:74
@@ +73,3 @@
+
+ diag(MemberCall->getLocStart(), "The shrink_to_fit method should be used "
+ "to reduce the capacity of a shrinkable "
----------------
s/The/the/
================
Comment at: clang-tidy/readability/ShrinkToFitCheck.cpp:79
@@ +78,3 @@
+ ReplacementText);
+ ;
+}
----------------
?
================
Comment at: clang-tidy/readability/ShrinkToFitCheck.h:18
@@ +17,3 @@
+
+/// \brief Replace copy and swap tricks on shrinkable containers with the \c
+/// shrink_to_fit() method call.
----------------
nit: I don't know whether Doxygen cares, but from the human point of view, \c
would make more sense next to the word it relates to.
================
Comment at: test/clang-tidy/readability-shrink-to-fit.cpp:15
@@ +14,3 @@
+
+ std::vector<int>& vref = v;
+ std::vector<int>(vref).swap(vref);
----------------
nit: Please put the "&" to the variable.
================
Comment at: test/clang-tidy/readability-shrink-to-fit.cpp:39
@@ +38,3 @@
+#define COPY_AND_SWAP_INT_VEC(x) std::vector<int>(x).swap(x)
+// CHECK-FIXES: #define COPY_AND_SWAP_INT_VEC(x) std::vector<int>(x).swap(x)
+
----------------
I don't know why the fix doesn't get applied here (which would be wrong). I
think, the check is avoiding this by pure coincidence. But it may be fine to
postpone solving this until someone stumbles upon the problem and has a good
test case.
================
Comment at: test/clang-tidy/readability-shrink-to-fit.cpp:47
@@ +46,3 @@
+ COPY_AND_SWAP_INT_VEC(v);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The shrink_to_fit method should
+}
----------------
Please add
// CHECK-FIXES: {{^ }}COPY_AND_SWAP_INT_VEC(v);{{$}}
to ensure this doesn't get spoiled.
http://reviews.llvm.org/D7087
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
