[clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stl containers.

[alexfh]

Summary

sizeof(some_std_string) is likely to be an error. This check finds this
pattern and suggests using .size() instead.

[alexfh]

Ignore template instantiations.

[aaron.ballman]

Great idea for a checker! Some comments below.

clang-tidy/misc/SizeofContainerCheck.cpp

37

Do you need the expr() matcher?

39

I think that this should be checking for more than just basic_string and vector. Why not other containers? For instance, anything that exposes a member function with a size() function seems like it would be reasonable.

49

I think the period should be replaced with a semicolon (it's not a sentence, so the period is also wrong). e.g. (with some wording tweaks),

"sizeof() does not return the size of the container; did you mean to call size()?"

Also, is this actually emitting the diagnostic? If so, a comment would be good explaining why the early return for macros is located where it is, so no one does a drive-by "fix."

[alexfh]

Match a broader set of containers. Updated diagnostic message. Added tests.

[alexfh]

clang-tidy/misc/SizeofContainerCheck.cpp

38

Yes, I do. sizeOfExpr().bind("...") doesn't compile for some reason.

tools/clang/tools/extra/clang-tidy/misc/SizeofContainerCheck.cpp:24:35: error: no member named 'bind' in 'clang::ast_matchers::internal::Matcher<clang::Stmt>'
                  .bind("expr"))).bind("sizeof"),
                  ~~~~~~~~~~~~~~~ ^

40

I'd like to limit this to the STL containers first. So "any recordDecl named std::.* with a .size() method" might work.

50

1. Done.
2. It's actually emitting the diagnostic. And I thought that the comment on line 52 below explains what happens in enough detail (// Don't generate fixes for macros.). If something is still unclear, can you explain what exactly?

[aaron.ballman]

I noticed a few more things, but mostly nitpicky at this point.

clang-tidy/misc/SizeofContainerCheck.cpp

23

Should we also ignore parens for when people do crazy things, like sizeof((((some_string+other_string))))?

26

I missed this earlier, but what about other 2-arg overloaded operators, like placement operator new & operator delete, or operator delete with size (and array forms)?

39

Why is "|::string" needed?

[alexfh]

Don't complain on the ARRAYSIZE(<array of containers>) pattern (where
sizeof(container) is used as a denominator).

[alexfh]

clang-tidy/misc/SizeofContainerCheck.cpp

23

I don't think we need to remove anything beyond the most external pair of parentheses.

26

Do you have an example of an expression that will break when a .size() is appended to it? Note, that it should be an expression of a class type.

39

Needed for code bases that use a std::string-like string class defined in the global namespace. Maybe we need a configuration option for custom container regexps. But this will likely be a follow up.

[aaron.ballman]

LGTM!

[aaron.ballman]

This appears to have broken one of the bots:

http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065

[alexfh]

Indeed. But this has been fixed before I could get to it.

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   InaccurateEraseCheck.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MoveConstructorInitCheck.cpp
   NoexceptMoveConstructorCheck.cpp
+  SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/misc/MiscTidyModule.cpp

 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
+#include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "misc-macro-parentheses");
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "misc-macro-repeated-side-effects");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
+    CheckFactories.registerCheck<SizeofContainerCheck>(
+        "misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "misc-static-assert");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
         "misc-uniqueptr-reset-release");

clang-tidy/misc/SizeofContainerCheck.h

+//===--- SizeofContainerCheck.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_MISC_SIZEOF_CONTAINER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SIZEOF_CONTAINER_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Find usages of sizeof on expressions of STL container types. Most likely the
+/// user wanted to use `.size()` instead.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-sizeof-container.html
+class SizeofContainerCheck : public ClangTidyCheck {
+public:
+  SizeofContainerCheck(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_MISC_SIZEOF_CONTAINER_H
+

clang-tidy/misc/SizeofContainerCheck.cpp

+//===--- SizeofContainerCheck.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 "SizeofContainerCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+namespace {
+
+bool needsParens(const Expr *E) {
+  E = E->IgnoreImpCasts();
+  if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
+    return true;
+  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E)) {
+    return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
+           Op->getOperator() != OO_Subscript;
+  }
+  return false;
+}
+
+} // anonymous namespace
+
+void SizeofContainerCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      expr(unless(isInTemplateInstantiation()),
+           expr(sizeOfExpr(has(expr(hasType(hasCanonicalType(hasDeclaration(
+                    recordDecl(matchesName("^(::std::|::string)"),
+                               hasMethod(methodDecl(hasName("size"), isPublic(),
+                                                    isConst()))))))))))
+               .bind("sizeof"),
+           // Ignore ARRAYSIZE(<array of containers>) pattern.
+           unless(hasAncestor(binaryOperator(
+               anyOf(hasOperatorName("/"), hasOperatorName("%")),
+               hasLHS(ignoringParenCasts(sizeOfExpr(expr()))),
+               hasRHS(ignoringParenCasts(equalsBoundNode("sizeof"))))))),
+      this);
+}
+
+void SizeofContainerCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *SizeOf =
+      Result.Nodes.getNodeAs<UnaryExprOrTypeTraitExpr>("sizeof");
+
+  SourceLocation SizeOfLoc = SizeOf->getLocStart();
+  auto Diag = diag(SizeOfLoc, "sizeof() doesn't return the size of the "
+                              "container; did you mean .size()?");
+
+  // Don't generate fixes for macros.
+  if (SizeOfLoc.isMacroID())
+    return;
+
+  SourceLocation RParenLoc = SizeOf->getRParenLoc();
+
+  // sizeof argument is wrapped in a single ParenExpr.
+  const auto *Arg = cast<ParenExpr>(SizeOf->getArgumentExpr());
+
+  if (needsParens(Arg->getSubExpr())) {
+    Diag << FixItHint::CreateRemoval(
+                CharSourceRange::getTokenRange(SizeOfLoc, SizeOfLoc))
+         << FixItHint::CreateInsertion(RParenLoc.getLocWithOffset(1),
+                                       ".size()");
+  } else {
+    Diag << FixItHint::CreateRemoval(
+                CharSourceRange::getTokenRange(SizeOfLoc, Arg->getLParen()))
+         << FixItHint::CreateReplacement(
+                CharSourceRange::getTokenRange(RParenLoc, RParenLoc),
+                ".size()");
+  }
+}
+
+} // namespace tidy
+} // namespace clang
+

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-assign-operator-signature
    misc-bool-pointer-implicit-conversion
    misc-inaccurate-erase
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-move-constructor-init
    misc-noexcept-move-constructor
+   misc-sizeof-container
    misc-static-assert
    misc-swapped-arguments
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii
    modernize-loop-convert
    modernize-pass-by-value
    modernize-replace-auto-ptr
    modernize-shrink-to-fit
    modernize-use-auto
    modernize-use-nullptr
    modernize-use-override
    readability-braces-around-statements
    readability-container-size-empty
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-inconsistent-declaration-parameter-name
    readability-named-parameter
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
-   readability-simplify-boolean-expr
+   readability-simplify-boolean-expr
 

docs/clang-tidy/checks/misc-sizeof-container.rst

+misc-sizeof-container
+=====================
+
+The check finds usages of ``sizeof`` on expressions of STL container types. Most
+likely the user wanted to use ``.size()`` instead.
+
+Currently only ``std::string`` and ``std::vector<T>`` are supported.
+
+Examples:
+
+.. code:: c++
+
+  std::string s;
+  int a = 47 + sizeof(s); // warning: sizeof() doesn't return the size of the container. Did you mean .size()?
+                          // The suggested fix is: int a = 47 + s.size();
+
+  int b = sizeof(std::string); // no warning, probably intended.

test/clang-tidy/misc-sizeof-container.cpp

+// RUN: %python %S/check_clang_tidy.py %s misc-sizeof-container %t
+
+namespace std {
+
+typedef unsigned int size_t;
+
+template <typename T>
+struct basic_string {
+  size_t size() const;
+};
+
+template <typename T>
+basic_string<T> operator+(const basic_string<T> &, const T *);
+
+typedef basic_string<char> string;
+
+template <typename T>
+struct vector {
+  size_t size() const;
+};
+
+class fake_container1 {
+  size_t size() const; // non-public
+};
+
+struct fake_container2 {
+  size_t size(); // non-const
+};
+
+}
+
+using std::size_t;
+
+#define ARRAYSIZE(a) \
+  ((sizeof(a) / sizeof(*(a))) / static_cast<size_t>(!(sizeof(a) % sizeof(*(a)))))
+
+#define ARRAYSIZE2(a) \
+  (((sizeof(a)) / (sizeof(*(a)))) / static_cast<size_t>(!((sizeof(a)) % (sizeof(*(a))))))
+
+struct string {
+  std::size_t size() const;
+};
+
+template<typename T>
+void g(T t) {
+  (void)sizeof(t);
+}
+
+void f() {
+  string s1;
+  std::string s2;
+  std::vector<int> v;
+
+  int a = 42 + sizeof(s1);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: sizeof() doesn't return the size of the container; did you mean .size()? [misc-sizeof-container]
+// CHECK-FIXES: int a = 42 + s1.size();
+  a = 123 * sizeof(s2);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: sizeof() doesn't return the size
+// CHECK-FIXES: a = 123 * s2.size();
+  a = 45 + sizeof(s2 + "asdf");
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: sizeof() doesn't return the size
+// CHECK-FIXES: a = 45 + (s2 + "asdf").size();
+  a = sizeof(v);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: sizeof() doesn't return the size
+// CHECK-FIXES: a = v.size();
+  a = sizeof(std::vector<int>{});
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: sizeof() doesn't return the size
+// CHECK-FIXES: a = std::vector<int>{}.size();
+
+  a = sizeof(a);
+  a = sizeof(int);
+  a = sizeof(std::string);
+  a = sizeof(std::vector<int>);
+
+  g(s1);
+  g(s2);
+  g(v);
+
+  std::fake_container1 f1;
+  std::fake_container2 f2;
+
+  a = sizeof(f1);
+  a = sizeof(f2);
+
+
+  std::string arr[3];
+  a = ARRAYSIZE(arr);
+  a = ARRAYSIZE2(arr);
+  a = sizeof(arr) / sizeof(arr[0]);
+
+  (void)a;
+}