Skip to content

Commit 2af9212

Browse files
JakubMelkaJakubMelka
committed
Ticket danmar#695: new style check : explicit declaration of ctor
1 parent 0131bda commit 2af9212

File tree

3 files changed

+136
-0
lines changed

3 files changed

+136
-0
lines changed

‎lib/checkclass.cpp‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,52 @@ void CheckClass::constructors()
217217
}
218218
}
219219

220+
void CheckClass::checkExplicitConstructors()
221+
{
222+
if (!_settings->isEnabled("performance"))
223+
return;
224+
225+
const std::size_t classes = symbolDatabase->classAndStructScopes.size();
226+
for (std::size_t i = 0; i < classes; ++i) {
227+
const Scope * scope = symbolDatabase->classAndStructScopes[i];
228+
229+
// Do not perform check, if the class/struct has not any constructors
230+
if (scope->numConstructors == 0)
231+
continue;
232+
233+
// Is class abstract? Maybe this test is over-simplification, but it will suffice for simple cases,
234+
// and it will avoid false positives.
235+
bool isAbstractClass = false;
236+
for (std::list<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
237+
if (func->isPure()) {
238+
isAbstractClass = true;
239+
break;
240+
}
241+
}
242+
243+
for (std::list<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
244+
245+
// We are looking for constructors, which are meeting following criteria:
246+
// 1) Constructor is declared with a single parameter
247+
// 2) Constructor is not declared as explicit
248+
// 3) It is not a copy/move constructor of non-abstract class
249+
// 4) Constructor is not marked as delete (programmer can mark the default constructor as deleted, which is ok)
250+
if (!func->isConstructor() || func->isDelete())
251+
continue;
252+
253+
if (!func->isExplicit() && func->argCount() == 1) {
254+
// We must decide, if it is not a copy/move constructor, or it is a copy/move constructor of abstract class.
255+
if (func->type != Function::eCopyConstructor && func->type != Function::eMoveConstructor) {
256+
noExplicitConstructorError(func->tokenDef, scope->className, scope->type == Scope::eStruct);
257+
}
258+
else if (isAbstractClass) {
259+
noExplicitCopyMoveConstructorError(func->tokenDef, scope->className, scope->type == Scope::eStruct);
260+
}
261+
}
262+
}
263+
}
264+
}
265+
220266
void CheckClass::copyconstructors()
221267
{
222268
if (!_settings->isEnabled("style"))
@@ -723,6 +769,16 @@ void CheckClass::noConstructorError(const Token *tok, const std::string &classna
723769
"instantiated. That may cause bugs or undefined behavior.");
724770
}
725771

772+
void CheckClass::noExplicitConstructorError(const Token *tok, const std::string &classname, bool isStruct)
773+
{
774+
reportError(tok, Severity::performance, "noExplicitConstructor", "The constructor of " + std::string(isStruct ? "struct" : "class") + " '" + classname + "' should be marked as explicit. ");
775+
}
776+
777+
void CheckClass::noExplicitCopyMoveConstructorError(const Token *tok, const std::string &classname, bool isStruct)
778+
{
779+
reportError(tok, Severity::performance, "noExplicitCopyMoveConstructor", "The copy/move constructor of abstract " + std::string(isStruct ? "struct" : "class") + " '" + classname + "' should be marked as explicit. ");
780+
}
781+
726782
void CheckClass::uninitVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive)
727783
{
728784
reportError(tok, Severity::warning, "uninitMemberVar", "Member variable '" + classname + "::" + varname + "' is not initialized in the constructor.", inconclusive);

‎lib/checkclass.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,17 @@ class CPPCHECKLIB CheckClass : public Check {
7676
checkClass.checkPureVirtualFunctionCall();
7777

7878
checkClass.checkDuplInheritedMembers();
79+
checkClass.checkExplicitConstructors();
7980
}
8081

8182

8283
/** @brief %Check that all class constructors are ok */
8384
void constructors();
8485

86+
/** @brief %Check that constructors with single parameter are explicit,
87+
* if they has to be.*/
88+
void checkExplicitConstructors();
89+
8590
/** @brief %Check that all private functions are called */
8691
void privateFunctions();
8792

@@ -136,6 +141,8 @@ class CPPCHECKLIB CheckClass : public Check {
136141

137142
// Reporting errors..
138143
void noConstructorError(const Token *tok, const std::string &classname, bool isStruct);
144+
void noExplicitConstructorError(const Token *tok, const std::string &classname, bool isStruct);
145+
void noExplicitCopyMoveConstructorError(const Token *tok, const std::string &classname, bool isStruct);
139146
//void copyConstructorMallocError(const Token *cctor, const Token *alloc, const std::string& var_name);
140147
void copyConstructorShallowCopyError(const Token *tok, const std::string& varname);
141148
void noCopyConstructorError(const Token *tok, const std::string &classname, bool isStruct);
@@ -165,6 +172,8 @@ class CPPCHECKLIB CheckClass : public Check {
165172
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
166173
CheckClass c(0, settings, errorLogger);
167174
c.noConstructorError(0, "classname", false);
175+
c.noExplicitConstructorError(0, "classname", false);
176+
c.noExplicitCopyMoveConstructorError(0, "classname", false);
168177
//c.copyConstructorMallocError(0, 0, "var");
169178
c.copyConstructorShallowCopyError(0, "var");
170179
c.noCopyConstructorError(0, "class", false);
@@ -199,6 +208,7 @@ class CPPCHECKLIB CheckClass : public Check {
199208
return "Check the code for each class.\n"
200209
"- Missing constructors and copy constructors\n"
201210
//"- Missing allocation of memory in copy constructor\n"
211+
"- Constructors which should be explicit are explicit.\n"
202212
"- Are all variables initialized by the constructors?\n"
203213
"- Are all variables assigned by 'operator='?\n"
204214
"- Warn if memset, memcpy etc are used on a class\n"

‎test/testclass.cpp‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,76 @@ class TestClass : public TestFixture {
180180
TEST_CASE(pureVirtualFunctionCallPrevented);
181181

182182
TEST_CASE(duplInheritedMembers);
183+
TEST_CASE(explicitConstructors);
184+
}
185+
186+
187+
void checkExplicitConstructors(const char code[]) {
188+
// Clear the error log
189+
errout.str("");
190+
Settings settings;
191+
settings.addEnabled("performance");
192+
193+
// Tokenize..
194+
Tokenizer tokenizer(&settings, this);
195+
std::istringstream istr(code);
196+
tokenizer.tokenize(istr, "test.cpp");
197+
tokenizer.simplifyTokenList2();
198+
199+
// Check..
200+
CheckClass checkClass(&tokenizer, &settings, this);
201+
checkClass.checkExplicitConstructors();
202+
}
203+
204+
void explicitConstructors() {
205+
checkExplicitConstructors("class Class \n"
206+
"{ \n"
207+
" Class() = delete; \n"
208+
" Class(const Class& other) { } \n"
209+
" Class(Class&& other) { } \n"
210+
" explicit Class(int i) { } \n"
211+
" explicit Class(const std::string&) { } \n"
212+
" Class(int a, int b) { } \n"
213+
"};");
214+
ASSERT_EQUALS("", errout.str());
215+
216+
checkExplicitConstructors("class Class \n"
217+
"{ \n"
218+
" Class() = delete; \n"
219+
" explicit Class(const Class& other) { } \n"
220+
" explicit Class(Class&& other) { } \n"
221+
" virtual int i() = 0; \n"
222+
"};");
223+
ASSERT_EQUALS("", errout.str());
224+
225+
checkExplicitConstructors("class Class \n"
226+
"{ \n"
227+
" Class() = delete; \n"
228+
" Class(const Class& other) = delete; \n"
229+
" Class(Class&& other) = delete; \n"
230+
" virtual int i() = 0; \n"
231+
"};");
232+
ASSERT_EQUALS("", errout.str());
233+
234+
checkExplicitConstructors("class Class \n"
235+
"{ \n"
236+
" Class(int i) { } \n"
237+
"};");
238+
ASSERT_EQUALS("[test.cpp:3]: (performance) The constructor of class 'Class' should be marked as explicit. \n", errout.str());
239+
240+
checkExplicitConstructors("class Class \n"
241+
"{ \n"
242+
" Class(const Class& other) { } \n"
243+
" virtual int i() = 0; \n"
244+
"};");
245+
ASSERT_EQUALS("[test.cpp:3]: (performance) The copy/move constructor of abstract class 'Class' should be marked as explicit. \n", errout.str());
246+
247+
checkExplicitConstructors("class Class \n"
248+
"{ \n"
249+
" Class(Class&& other) { } \n"
250+
" virtual int i() = 0; \n"
251+
"};");
252+
ASSERT_EQUALS("[test.cpp:3]: (performance) The copy/move constructor of abstract class 'Class' should be marked as explicit. \n", errout.str());
183253
}
184254

185255
void checkDuplInheritedMembers(const char code[]) {

0 commit comments

Comments
 (0)