office-gobmx/compilerplugins/clang/store/badvectorinit.cxx
Luboš Luňák bef96f7a7b better name for a function in compilerplugins
The function is not just about a spelling location.

Change-Id: I96e9e9ef7e27a9763397b4b86473c1c30d0e3eeb
Reviewed-on: https://gerrit.libreoffice.org/80381
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
2019-10-08 20:46:36 +02:00

214 lines
6.6 KiB
C++

/* -*- 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: */