1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
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"<--- Same expression on both sides of '||'. [+]Finding the same expression on both sides of an operator is suspicious and might indicate a cut and paste or logic error. Please examine this code carefully to determine if it is correct.
|| aFileName == SRCDIR "/vcl/source/gdi/gdimtf.cxx"<--- Same expression on both sides of '||'. [+]Finding the same expression on both sides of an operator is suspicious and might indicate a cut and paste or logic error. Please examine this code carefully to determine if it is correct.
)
{
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: */
|