compilerplugins/clang/store/badvectorinit.cxx

   1 /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
   2 /*
   3  * This file is part of the LibreOffice project.
   4  *
   5  * This Source Code Form is subject to the terms of the Mozilla Public
   6  * License, v. 2.0. If a copy of the MPL was not distributed with this
   7  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
   8  */
   9
  10 #include <cassert>
  11 #include <string>
  12 #include <iostream>
  13 #include <fstream>
  14 #include <set>
  15 #include "plugin.hxx"
  16
  17 /**
  18
  19 Comments from Bjoern Michaelsen:
  20
  21 Killing the 1-argument vector fill constructor:
  22
  23  std::vector< basebmp::Color > aDevPal(2);
  24
  25 in general is probably a Good Thing(tm). It can just be too misleading.
  26 Requiring at least the explicit two-value fill constructor for the rare cases where
  27 someone wants a filled vector isn't too much to ask and less prone to
  28 misunderstandings:
  29
  30  std::vector< basebmp::Color > aDevPal(2, basebmp::Color(0,0,0));
  31
  32 Although that _still_ might be misleading[1], so turning all those into the
  33 somewhat longer, but more explicit:
  34
  35  std::vector< basebmp::Color > aDevPal;
  36  aDevPal.reserve(2);
  37  aDevPal.push_back(...);
  38  ...
  39
  40 > So I suppose the check would be for a size reservation on a vector
  41 > followed by push_back - rather than some array indexing - does that make
  42 > sense ? or did I go crazy ;-)
  43
  44 Yes, in general you want neither of the above forms. Preferably instead of
  45 e.g.:
  46
  47  std::vector< basebmp::Color > aDevPal(2);
  48  aDevPal[0] = basebmp::Color( 0, 0, 0 );
  49  aDevPal[1] = basebmp::Color( 0xff, 0xff, 0xff );
  50
  51 you would -- if possible -- simply:
  52
  53  std::vector< basebmp::Color > aDevPal{
  54     basebmp::Color( 0, 0, 0 ),
  55     basebmp::Color( 0xff, 0xff, 0xff ) };
  56
  57 and only for complex cases, where you do not have the elements statically
  58 available, something like:
  59
  60  std::vector< foo > vFoos;
  61  vFoos.reserve(vInput.size());
  62  std::transform(std::back_inserter(vFoos),
  63      vInput.begin(),
  64      vInput.end(),
  65      [] (decltype(vInput)::value_type aInputValue) { return do_something(aInputValue); });
  66
  67 see also:
  68 https://skyfromme.wordpress.com/2015/03/02/50-ways-to-fill-your-vector/
  69 https://skyfromme.wordpress.com/2015/03/12/following-the-white-rabbit/
  70 (tl;dr: Use initializer lists to fill vectors when possible).
  71
  72 Best,
  73
  74 Bjoern
  75
  76 [1] Well, except that:
  77      std::vector<int>(3, 0)
  78     is doing something different from:
  79      std::vector<int>{3, 0}
  80     just to make things more interesting. But hey, that's C++ for you.
  81     But that wart exists for the 1-arg ctor too -- yet another reason to kill that.
  82 */
  83
  84 namespace {
  85
  86
  87 class BadVectorInit:
  88     public loplugin::FilteringPlugin<BadVectorInit>
  89 {
  90 public:
  91     explicit BadVectorInit(InstantiationData const & data): FilteringPlugin(data) {}
  92
  93     virtual void run() override
  94     {
  95         TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
  96     }
  97
  98     bool VisitCXXConstructExpr(const CXXConstructExpr* );
  99     bool TraverseFunctionDecl(FunctionDecl* );
 100     bool VisitCXXMemberCallExpr(const CXXMemberCallExpr* );
 101 private:
 102     StringRef getFilename(SourceLocation loc);
 103     std::set<const VarDecl*> suspectSet;
 104 };
 105
 106 bool BadVectorInit::TraverseFunctionDecl(FunctionDecl* decl)
 107 {
 108     bool ret = RecursiveASTVisitor::TraverseFunctionDecl(decl);
 109     suspectSet.clear();
 110     return ret;
 111 }
 112
 113 StringRef BadVectorInit::getFilename(SourceLocation loc)
 114 {
 115     SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(loc);
 116     StringRef name { compiler.getSourceManager().getFilename(spellingLocation) };
 117     return name;
 118 }
 119
 120 bool BadVectorInit::VisitCXXMemberCallExpr(const CXXMemberCallExpr* expr)
 121 {
 122     if (suspectSet.empty() || ignoreLocation( expr ))
 123         return true;
 124
 125     // need to exclude some false positives
 126     StringRef aFileName = getFilename(expr->getLocStart());
 127     if (aFileName == SRCDIR "/framework/source/services/autorecovery.cxx"
 128         || aFileName == SRCDIR "/vcl/source/opengl/OpenGLHelper.cxx"
 129         || aFileName == SRCDIR "/vcl/source/gdi/gdimtf.cxx"
 130         )
 131     {
 132         return true;
 133     }
 134
 135     const FunctionDecl* functionDecl = expr->getDirectCallee();
 136     if (!functionDecl)
 137         return true;
 138     if (functionDecl->getNameAsString().find("push_back") == string::npos)
 139         return true;
 140     const DeclRefExpr* declExpr = dyn_cast<DeclRefExpr>(expr->getImplicitObjectArgument());
 141     if (!declExpr)
 142         return true;
 143     const VarDecl* varDecl = dyn_cast<VarDecl>(declExpr->getDecl());
 144     if (!varDecl)
 145         return true;
 146     varDecl = varDecl->getCanonicalDecl();
 147     if (suspectSet.find(varDecl) == suspectSet.end())
 148         return true;
 149     report(
 150         DiagnosticsEngine::Warning,
 151         "calling push_back after using sized constructor",
 152         expr->getLocStart())
 153         << expr->getSourceRange();
 154     report(
 155         DiagnosticsEngine::Note,
 156         "on this var",
 157         varDecl->getLocStart())
 158         << varDecl->getSourceRange();
 159
 160     return true;
 161 }
 162
 163 bool BadVectorInit::VisitCXXConstructExpr(const CXXConstructExpr* expr)
 164 {
 165     if (ignoreLocation( expr ))
 166         return true;
 167
 168     const CXXConstructorDecl *consDecl = expr->getConstructor();
 169     consDecl = consDecl->getCanonicalDecl();
 170
 171     // The default constructor can potentially have a parameter, e.g.
 172     // in glibcxx-debug the default constructor is:
 173     // explicit vector(const _Allocator& __a = _Allocator())
 174     if (consDecl->param_size() == 0 || consDecl->isDefaultConstructor())
 175         return true;
 176
 177     std::string aParentName = consDecl->getParent()->getQualifiedNameAsString();
 178     if (aParentName.find("vector") == string::npos && aParentName.find("deque") == string::npos)
 179         return true;
 180
 181     // ignore the copy/move constructors, and those taking an initializer_list
 182     // etc.:
 183     if (consDecl->isCopyConstructor() || consDecl->isMoveConstructor())
 184         return true;
 185     const ParmVarDecl* pParam = consDecl->getParamDecl(0);
 186     std::string aParam1 = pParam->getOriginalType().getAsString();
 187     if (aParam1.find("initializer_list") != string::npos
 188         || aParam1.find("iterator") != string::npos)
 189         return true;
 190
 191      // found a call to the 1-arg vector constructor, now look for the VarDecl it belongs to
 192
 193      const Stmt* parent = expr;
 194      do {
 195          parent = parentStmt(parent);
 196          if (!parent) break;
 197          if (isa<DeclStmt>(parent))
 198          {
 199              const DeclStmt* declStmt = dyn_cast<DeclStmt>(parent);
 200              const Decl* decl = declStmt->getSingleDecl();
 201              if (decl && isa<VarDecl>(decl))
 202                  suspectSet.insert(dyn_cast<VarDecl>(decl)->getCanonicalDecl());
 203              break;
 204          }
 205      } while (true);
 206
 207     return true;
 208 }
 209
 210 loplugin::Plugin::Registration< BadVectorInit > X("badvectorinit", true);
 211
 212 }
 213
 214 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */