[Clang Tidy]: Use shrink_to_fit instead of copy and swap trick.

[xazax.hun]

Summary

The \c shrink_to_fit() method is more readable and more effective than
the copy and swap trick to reduce the capacity of a shrinkable container.
Note that, the \c shrink_to_fit() method is only available in C++11 and up.

Example:

std::vector<int>(v).swap(v); will be replaced with v.shrink_to_fit();

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?

[alexfh]

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.

clang-tidy/readability/ShrinkToFitCheck.cpp

49

Maybe also handle pointer-type variables/data members?

72

Please use Result.Context->getLangOpts() instead (I've fixed the other two checks using LangOptions()).

75

s/The/the/

80

?

clang-tidy/readability/ShrinkToFitCheck.h

19

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.

test/clang-tidy/readability-shrink-to-fit.cpp

16

nit: Please put the "&" to the variable.

40

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.

48

Please add

// CHECK-FIXES: {{^  }}COPY_AND_SWAP_INT_VEC(v);{{$}}

to ensure this doesn't get spoiled.

[xazax.hun]

Thank you for the review!

• I have modified the doxygen documentation as you recommended.
• The actual language options are used.
• No warnings should be generated for files that compiled without C++11 support (although I am not sure that this method works for headers in general).
• Fixed the warning string and removed the redundant empty statement.
• Do not try to fix code that is the result of a macro expansion (I am not sure that my solution is idiomatic).
• Added pointer support.
• Fixed some style issues.

[alexfh]

clang-tidy/readability/ShrinkToFitCheck.cpp

52

nit: It looks like there's one pair of parentheses more than needed. Also below on lines 57-59.

test/clang-tidy/readability-shrink-to-fit.cpp

47

Can you add one more test?

in g():

...
T v2;
T(v2).swap(v2);

in h():

g<std::vector<int>>();

[xazax.hun]

Thank you for pointing out the issue. It looks like not matching on instantiations solves the problem.

[alexfh]

Looks good with a couple of nits. See the comments.

test/clang-tidy/readability-shrink-to-fit.cpp

48

Any reason for this to be a separate function? Why not just add a test with v2 to g<>?

67

nit: In C++11 you don't need to put a space here.

[alexfh]

test/clang-tidy/readability-shrink-to-fit.cpp

48

Ah, I get it. No need to address this ;)

67

I'll fix it myself and commit the patch.

[alexfh]

Committed revision 226912.

Modified before commit:

• removed a space between >s
• added a test for dependent types in templates.

clang-tidy/readability/CMakeLists.txt

   NamespaceCommentCheck.cpp
   ReadabilityTidyModule.cpp
   RedundantSmartptrGet.cpp
+  ShrinkToFitCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "ElseAfterReturnCheck.h"
 #include "FunctionSize.h"
 #include "RedundantSmartptrGet.h"
+#include "ShrinkToFitCheck.h"
 
 namespace clang {
 namespace tidy {

         "readability-function-size");
     CheckFactories.registerCheck<RedundantSmartptrGet>(
         "readability-redundant-smartptr-get");
+    CheckFactories.registerCheck<ShrinkToFitCheck>(
+        "readability-shrink-to-fit");
   }
 };
 

clang-tidy/readability/ShrinkToFitCheck.h

+//===--- ShrinkToFitCheck.h - clang-tidy-------------------------*- C++ -*-===//
+//
+//                     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_READABILITY_SHRINK_TO_FIT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SHRINK_TO_FIT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Replace copy and swap tricks on shrinkable containers with the
+/// \c shrink_to_fit() method call.
+///
+/// The \c shrink_to_fit() method is more readable and more effective than
+/// the copy and swap trick to reduce the capacity of a shrinkable container.
+/// Note that, the \c shrink_to_fit() method is only available in C++11 and up.
+class ShrinkToFitCheck : public ClangTidyCheck {
+public:
+  ShrinkToFitCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_SHRINK_TO_FIT_H

clang-tidy/readability/ShrinkToFitCheck.cpp

+//===--- ShrinkToFitCheck.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 "ShrinkToFitCheck.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace {
+bool isShrinkableContainer(llvm::StringRef ClassName) {
+  static const llvm::StringSet<> Shrinkables = [] {
+    llvm::StringSet<> RetVal;
+    RetVal.insert("std::deque");
+    RetVal.insert("std::basic_string");
+    RetVal.insert("std::vector");
+    return RetVal;
+  }();
+  return Shrinkables.find(ClassName) != Shrinkables.end();
+}
+}
+
+namespace clang {
+namespace ast_matchers {
+AST_MATCHER(NamedDecl, stlShrinkableContainer) {
+  return isShrinkableContainer(Node.getQualifiedNameAsString());
+}
+} // namespace ast_matchesr
+} // namespace clang
+
+namespace clang {
+namespace tidy {
+
+void ShrinkToFitCheck::registerMatchers(MatchFinder *Finder) {
+  // Swap as a function need not to be considered, because rvalue can not
+  // be bound to a non-const reference.
+  const auto ShrinkableAsMember =
+      memberExpr(member(valueDecl().bind("ContainerDecl")));
+  const auto ShrinkableAsDecl =
+      declRefExpr(hasDeclaration(valueDecl().bind("ContainerDecl")));
+  const auto CopyCtorCall = constructExpr(
+      hasArgument(0, anyOf(ShrinkableAsMember, ShrinkableAsDecl,
+                           unaryOperator(has(ShrinkableAsMember)),
+                           unaryOperator(has(ShrinkableAsDecl)))));
+  const auto SwapParam = expr(anyOf(
+      memberExpr(member(equalsBoundNode("ContainerDecl"))),
+      declRefExpr(hasDeclaration(equalsBoundNode("ContainerDecl"))),
+      unaryOperator(has(memberExpr(member(equalsBoundNode("ContainerDecl"))))),
+      unaryOperator(
+          has(declRefExpr(hasDeclaration(equalsBoundNode("ContainerDecl")))))));
+
+  Finder->addMatcher(
+      memberCallExpr(on(hasType(namedDecl(stlShrinkableContainer()))),
+                     callee(methodDecl(hasName("swap"))),
+                     has(memberExpr(hasDescendant(CopyCtorCall))),
+                     hasArgument(0, SwapParam.bind("ContainerToShrink")),
+                     unless(isInTemplateInstantiation()))
+          .bind("CopyAndSwapTrick"),
+      this);
+}
+
+void ShrinkToFitCheck::check(const MatchFinder::MatchResult &Result) {
+  const LangOptions &Opts = Result.Context->getLangOpts();
+
+  if (!Opts.CPlusPlus11)
+    return;
+
+  const auto *MemberCall =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("CopyAndSwapTrick");
+  const auto *Container = Result.Nodes.getNodeAs<Expr>("ContainerToShrink");
+  FixItHint Hint;
+
+  if (!MemberCall->getLocStart().isMacroID()) {
+    std::string ReplacementText;
+    if (const auto *UnaryOp = llvm::dyn_cast<UnaryOperator>(Container)) {
+      ReplacementText =
+          Lexer::getSourceText(CharSourceRange::getTokenRange(
+                                   UnaryOp->getSubExpr()->getSourceRange()),
+                               *Result.SourceManager, Opts);
+      ReplacementText += "->shrink_to_fit()";
+    } else {
+      ReplacementText = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(Container->getSourceRange()),
+          *Result.SourceManager, Opts);
+      ReplacementText += ".shrink_to_fit()";
+    }
+
+    Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(),
+                                        ReplacementText);
+  }
+
+  diag(MemberCall->getLocStart(), "the shrink_to_fit method should be used "
+                                  "to reduce the capacity of a shrinkable "
+                                  "container")
+      << Hint;
+}
+
+} // namespace tidy
+} // namespace clang

test/clang-tidy/readability-shrink-to-fit.cpp

+// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-shrink-to-fit %t
+// REQUIRES: shell
+
+namespace std {
+template <typename T> struct vector { void swap(vector &other); };
+}
+
+void f() {
+  std::vector<int> v;
+
+  std::vector<int>(v).swap(v);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the shrink_to_fit method should be used to reduce the capacity of a shrinkable container [readability-shrink-to-fit] 
+  // CHECK-FIXES: {{^  }}v.shrink_to_fit();{{$}}
+
+  std::vector<int> &vref = v;
+  std::vector<int>(vref).swap(vref);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the shrink_to_fit method should
+  // CHECK-FIXES: {{^  }}vref.shrink_to_fit();{{$}}
+
+  std::vector<int> *vptr = &v;
+  std::vector<int>(*vptr).swap(*vptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the shrink_to_fit method should
+  // CHECK-FIXES: {{^  }}vptr->shrink_to_fit();{{$}}
+}
+
+struct X {
+  std::vector<int> v;
+  void f() {
+    std::vector<int>(v).swap(v);
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: the shrink_to_fit method should
+    // CHECK-FIXES: {{^    }}v.shrink_to_fit();{{$}}
+
+    std::vector<int> *vptr = &v;
+    std::vector<int>(*vptr).swap(*vptr);
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: the shrink_to_fit method should
+    // CHECK-FIXES: {{^    }}vptr->shrink_to_fit();{{$}}
+  }
+};
+
+template <typename T> void g() {
+  std::vector<int> v;
+
+  std::vector<int>(v).swap(v);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the shrink_to_fit method should 
+  // CHECK-FIXES: {{^  }}v.shrink_to_fit();{{$}}
+}
+
+template <typename T> void g2() {
+  T v2;
+  T(v2).swap(v2);
+  // CHECK-FIXES: {{^  }}T(v2).swap(v2);{{$}}
+
+  std::vector<int> v;
+
+  std::vector<int>(v).swap(v);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the shrink_to_fit method should 
+  // CHECK-FIXES: {{^  }}v.shrink_to_fit();{{$}}
+}
+
+#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)
+
+void h() {
+  g<int>();
+  g<double>();
+  g<bool>();
+  g2<std::vector<int> >();
+  std::vector<int> v;
+  COPY_AND_SWAP_INT_VEC(v);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the shrink_to_fit method should 
+  // CHECK-FIXES: {{^  }}COPY_AND_SWAP_INT_VEC(v);{{$}}
+}
+