diff options
Diffstat (limited to 'compilerplugins/clang/store/badvectorinit.cxx')
| -rw-r--r-- | compilerplugins/clang/store/badvectorinit.cxx | 214 |
1 files changed, 214 insertions, 0 deletions
diff --git a/compilerplugins/clang/store/badvectorinit.cxx b/compilerplugins/clang/store/badvectorinit.cxx new file mode 100644 index 000000000..4ab086d9e --- /dev/null +++ b/compilerplugins/clang/store/badvectorinit.cxx @@ -0,0 +1,214 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#include <cassert> +#include <string> +#include <iostream> +#include <fstream> +#include <set> +#include "plugin.hxx" + +/** + +Comments from Bjoern Michaelsen: + +Killing the 1-argument vector fill constructor: + + std::vector< basebmp::Color > aDevPal(2); + +in general is probably a Good Thing(tm). It can just be too misleading. +Requiring at least the explicit two-value fill constructor for the rare cases where +someone wants a filled vector isn't too much to ask and less prone to +misunderstandings: + + std::vector< basebmp::Color > aDevPal(2, basebmp::Color(0,0,0)); + +Although that _still_ might be misleading[1], so turning all those into the +somewhat longer, but more explicit: + + std::vector< basebmp::Color > aDevPal; + aDevPal.reserve(2); + aDevPal.push_back(...); + ... + +> So I suppose the check would be for a size reservation on a vector +> followed by push_back - rather than some array indexing - does that make +> sense ? or did I go crazy ;-) + +Yes, in general you want neither of the above forms. Preferably instead of +e.g.: + + std::vector< basebmp::Color > aDevPal(2); + aDevPal[0] = basebmp::Color( 0, 0, 0 ); + aDevPal[1] = basebmp::Color( 0xff, 0xff, 0xff ); + +you would -- if possible -- simply: + + std::vector< basebmp::Color > aDevPal{ + basebmp::Color( 0, 0, 0 ), + basebmp::Color( 0xff, 0xff, 0xff ) }; + +and only for complex cases, where you do not have the elements statically +available, something like: + + std::vector< foo > vFoos; + vFoos.reserve(vInput.size()); + std::transform(std::back_inserter(vFoos), + vInput.begin(), + vInput.end(), + [] (decltype(vInput)::value_type aInputValue) { return do_something(aInputValue); }); + +see also: +https://skyfromme.wordpress.com/2015/03/02/50-ways-to-fill-your-vector/ +https://skyfromme.wordpress.com/2015/03/12/following-the-white-rabbit/ +(tl;dr: Use initializer lists to fill vectors when possible). + +Best, + +Bjoern + +[1] Well, except that: + std::vector<int>(3, 0) + is doing something different from: + std::vector<int>{3, 0} + just to make things more interesting. But hey, that's C++ for you. + But that wart exists for the 1-arg ctor too -- yet another reason to kill that. +*/ + +namespace { + + +class BadVectorInit: + public loplugin::FilteringPlugin<BadVectorInit> +{ +public: + explicit BadVectorInit(InstantiationData const & data): FilteringPlugin(data) {} + + virtual void run() override + { + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + } + + bool VisitCXXConstructExpr(const CXXConstructExpr* ); + bool TraverseFunctionDecl(FunctionDecl* ); + bool VisitCXXMemberCallExpr(const CXXMemberCallExpr* ); +private: + StringRef getFilename(SourceLocation loc); + std::set<const VarDecl*> suspectSet; +}; + +bool BadVectorInit::TraverseFunctionDecl(FunctionDecl* decl) +{ + bool ret = RecursiveASTVisitor::TraverseFunctionDecl(decl); + suspectSet.clear(); + return ret; +} + +StringRef BadVectorInit::getFilename(SourceLocation loc) +{ + SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(loc); + StringRef name { getFilenameOfLocation(spellingLocation) }; + return name; +} + +bool BadVectorInit::VisitCXXMemberCallExpr(const CXXMemberCallExpr* expr) +{ + if (suspectSet.empty() || ignoreLocation( expr )) + return true; + + // need to exclude some false positives + StringRef aFileName = getFilename(expr->getLocStart()); + if (aFileName == SRCDIR "/framework/source/services/autorecovery.cxx" + || aFileName == SRCDIR "/vcl/source/opengl/OpenGLHelper.cxx" + || aFileName == SRCDIR "/vcl/source/gdi/gdimtf.cxx" + ) + { + return true; + } + + const FunctionDecl* functionDecl = expr->getDirectCallee(); + if (!functionDecl) + return true; + if (functionDecl->getNameAsString().find("push_back") == string::npos) + return true; + const DeclRefExpr* declExpr = dyn_cast<DeclRefExpr>(expr->getImplicitObjectArgument()); + if (!declExpr) + return true; + const VarDecl* varDecl = dyn_cast<VarDecl>(declExpr->getDecl()); + if (!varDecl) + return true; + varDecl = varDecl->getCanonicalDecl(); + if (suspectSet.find(varDecl) == suspectSet.end()) + return true; + report( + DiagnosticsEngine::Warning, + "calling push_back after using sized constructor", + expr->getLocStart()) + << expr->getSourceRange(); + report( + DiagnosticsEngine::Note, + "on this var", + varDecl->getLocStart()) + << varDecl->getSourceRange(); + + return true; +} + +bool BadVectorInit::VisitCXXConstructExpr(const CXXConstructExpr* expr) +{ + if (ignoreLocation( expr )) + return true; + + const CXXConstructorDecl *consDecl = expr->getConstructor(); + consDecl = consDecl->getCanonicalDecl(); + + // The default constructor can potentially have a parameter, e.g. + // in glibcxx-debug the default constructor is: + // explicit vector(const _Allocator& __a = _Allocator()) + if (consDecl->param_size() == 0 || consDecl->isDefaultConstructor()) + return true; + + std::string aParentName = consDecl->getParent()->getQualifiedNameAsString(); + if (aParentName.find("vector") == string::npos && aParentName.find("deque") == string::npos) + return true; + + // ignore the copy/move constructors, and those taking an initializer_list + // etc.: + if (consDecl->isCopyConstructor() || consDecl->isMoveConstructor()) + return true; + const ParmVarDecl* pParam = consDecl->getParamDecl(0); + std::string aParam1 = pParam->getOriginalType().getAsString(); + if (aParam1.find("initializer_list") != string::npos + || aParam1.find("iterator") != string::npos) + return true; + + // found a call to the 1-arg vector constructor, now look for the VarDecl it belongs to + + const Stmt* parent = expr; + do { + parent = parentStmt(parent); + if (!parent) break; + if (isa<DeclStmt>(parent)) + { + const DeclStmt* declStmt = dyn_cast<DeclStmt>(parent); + const Decl* decl = declStmt->getSingleDecl(); + if (decl && isa<VarDecl>(decl)) + suspectSet.insert(dyn_cast<VarDecl>(decl)->getCanonicalDecl()); + break; + } + } while (true); + + return true; +} + +loplugin::Plugin::Registration< BadVectorInit > X("badvectorinit", true); + +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ |