Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #13282 FN functionConst with unknown base class #6990

Merged
merged 23 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cfg/qt.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -5359,7 +5359,7 @@
<define name="Q_LIKELY(expr)" value="expr"/>
<define name="Q_NAMESPACE" value=""/>
<define name="Q_NULLPTR" value="NULL"/>
<define name="Q_OBJECT" value="static void qt_static_metacall(QObject*,QMetaObject::Call,int,void**);const MetaObject* metaObject() const;void* qt_metacast(const char*);int qt_metacall(QMetaObject::Call,int,void**);"/>
<define name="Q_OBJECT" value="static void qt_static_metacall(QObject*,QMetaObject::Call,int,void**);virtual const MetaObject* metaObject() const;virtual void* qt_metacast(const char*);virtual int qt_metacall(QMetaObject::Call,int,void**);"/>
<define name="Q_PRIVATE_SLOT(d, signature)" value=""/>
<define name="Q_SLOTS" value=""/>
<!-- Treat as variadic macro to avoid preprocessorErrorDirective -->
Expand Down
4 changes: 2 additions & 2 deletions gui/codeeditstylecontrols.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ void SelectColorButton::setColor(const QColor& color)
}

// cppcheck-suppress unusedFunction
const QColor& SelectColorButton::getColor()
const QColor& SelectColorButton::getColor() const
{
return mColor;
}
Expand Down Expand Up @@ -122,7 +122,7 @@ void SelectFontWeightCombo::setWeight(QFont::Weight weight)
}

// cppcheck-suppress unusedFunction
const QFont::Weight& SelectFontWeightCombo::getWeight()
const QFont::Weight& SelectFontWeightCombo::getWeight() const
{
return mWeight;
}
4 changes: 2 additions & 2 deletions gui/codeeditstylecontrols.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class SelectColorButton : public QPushButton {
explicit SelectColorButton(QWidget* parent);

void setColor(const QColor& color);
const QColor& getColor();
const QColor& getColor() const;

signals:
// NOLINTNEXTLINE(readability-inconsistent-declaration-parameter-name) - caused by generated MOC code
Expand All @@ -57,7 +57,7 @@ class SelectFontWeightCombo : public QComboBox {
explicit SelectFontWeightCombo(QWidget* parent);

void setWeight(QFont::Weight weight);
const QFont::Weight& getWeight();
const QFont::Weight& getWeight() const;

signals:
// NOLINTNEXTLINE(readability-inconsistent-declaration-parameter-name) - caused by generated MOC code
Expand Down
2 changes: 1 addition & 1 deletion gui/codeeditstyledialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ void StyleEditDialog::updateStyle()
mSampleEditor->setStyle(mStyleOutgoing);
}

CodeEditorStyle StyleEditDialog::getStyle()
CodeEditorStyle StyleEditDialog::getStyle() const
{
return mStyleOutgoing;
}
Expand Down
2 changes: 1 addition & 1 deletion gui/codeeditstyledialog.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class StyleEditDialog : public QDialog {
explicit StyleEditDialog(const CodeEditorStyle& newStyle,
QWidget *parent = nullptr);

CodeEditorStyle getStyle();
CodeEditorStyle getStyle() const;

private:
void updateControls();
Expand Down
2 changes: 1 addition & 1 deletion gui/resultstree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,7 @@ void ResultsTree::setCheckDirectory(const QString &dir)
}


const QString& ResultsTree::getCheckDirectory()
const QString& ResultsTree::getCheckDirectory() const
{
return mCheckPath;
}
Expand Down
2 changes: 1 addition & 1 deletion gui/resultstree.h
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ class ResultsTree : public QTreeView {
* @return Directory containing source files
*/

const QString& getCheckDirectory();
const QString& getCheckDirectory() const;

/**
* @brief Check if there are any visible results in view.
Expand Down
4 changes: 2 additions & 2 deletions gui/resultsview.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ void ResultsView::printPreview()
dialog.exec();
}

void ResultsView::print(QPrinter* printer)
void ResultsView::print(QPrinter* printer) const
{
if (!hasResults()) {
QMessageBox msgBox;
Expand Down Expand Up @@ -300,7 +300,7 @@ void ResultsView::setCheckDirectory(const QString &dir)
mUI->mTree->setCheckDirectory(dir);
}

QString ResultsView::getCheckDirectory()
QString ResultsView::getCheckDirectory() const
{
return mUI->mTree->getCheckDirectory();
}
Expand Down
4 changes: 2 additions & 2 deletions gui/resultsview.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class ResultsView : public QWidget {
* @return Directory containing source files
*/

QString getCheckDirectory();
QString getCheckDirectory() const;

/**
* Set settings used in checking
Expand Down Expand Up @@ -331,7 +331,7 @@ public slots:
* @brief Slot printing the current report to the printer.
* @param printer The printer used for printing the report.
*/
void print(QPrinter* printer);
void print(QPrinter* printer) const;

/**
* @brief Slot opening a print preview dialog
Expand Down
36 changes: 24 additions & 12 deletions lib/checkclass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2176,7 +2176,8 @@ void CheckClass::checkConst()
}

// check if base class function is virtual
if (!scope->definedType->derivedFrom.empty() && func.isImplicitlyVirtual(true))
bool foundAllBaseClasses = true;
if (!scope->definedType->derivedFrom.empty() && func.isImplicitlyVirtual(true, &foundAllBaseClasses) && foundAllBaseClasses)
continue;

MemberAccess memberAccessed = MemberAccess::NONE;
Expand Down Expand Up @@ -2204,9 +2205,9 @@ void CheckClass::checkConst()
functionName += "]";

if (func.isInline())
checkConstError(func.token, classname, functionName, suggestStatic);
checkConstError(func.token, classname, functionName, suggestStatic, foundAllBaseClasses);
else // not inline
checkConstError2(func.token, func.tokenDef, classname, functionName, suggestStatic);
checkConstError2(func.token, func.tokenDef, classname, functionName, suggestStatic, foundAllBaseClasses);
}
}
}
Expand Down Expand Up @@ -2281,16 +2282,21 @@ bool CheckClass::isMemberVar(const Scope *scope, const Token *tok) const
// not found in this class
if (!scope->definedType->derivedFrom.empty()) {
// check each base class
bool foundAllBaseClasses = true;
for (const Type::BaseInfo & i : scope->definedType->derivedFrom) {
// find the base class
const Type *derivedFrom = i.type;
if (!derivedFrom)
foundAllBaseClasses = false;

// find the function in the base class
if (derivedFrom && derivedFrom->classScope && derivedFrom->classScope != scope) {
if (isMemberVar(derivedFrom->classScope, tok))
return true;
}
}
if (!foundAllBaseClasses)
return true;
}

return false;
Expand Down Expand Up @@ -2613,35 +2619,41 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, Member
return true;
}

void CheckClass::checkConstError(const Token *tok, const std::string &classname, const std::string &funcname, bool suggestStatic)
void CheckClass::checkConstError(const Token *tok, const std::string &classname, const std::string &funcname, bool suggestStatic, bool foundAllBaseClasses)
{
checkConstError2(tok, nullptr, classname, funcname, suggestStatic);
checkConstError2(tok, nullptr, classname, funcname, suggestStatic, foundAllBaseClasses);
}

void CheckClass::checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname, bool suggestStatic)
void CheckClass::checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname, bool suggestStatic, bool foundAllBaseClasses)
{
std::list<const Token *> toks{ tok1 };
if (tok2)
toks.push_back(tok2);
if (!suggestStatic)
if (!suggestStatic) {
const std::string msg = foundAllBaseClasses ?
"Technically the member function '$symbol' can be const.\nThe member function '$symbol' can be made a const " :
"Either there is a missing 'override', or the member function '$symbol' can be const.\nUnless it overrides a base class member, the member function '$symbol' can be made a const ";
reportError(toks, Severity::style, "functionConst",
"$symbol:" + classname + "::" + funcname +"\n"
"Technically the member function '$symbol' can be const.\n"
"The member function '$symbol' can be made a const "
+ msg +
"function. Making this function 'const' should not cause compiler errors. "
"Even though the function can be made const function technically it may not make "
"sense conceptually. Think about your design and the task of the function first - is "
"it a function that must not change object internal state?", CWE398, Certainty::inconclusive);
else
}
else {
const std::string msg = foundAllBaseClasses ?
"Technically the member function '$symbol' can be static (but you may consider moving to unnamed namespace).\nThe member function '$symbol' can be made a static " :
"Either there is a missing 'override', or the member function '$symbol' can be static.\nUnless it overrides a base class member, the member function '$symbol' can be made a static ";
reportError(toks, Severity::performance, "functionStatic",
"$symbol:" + classname + "::" + funcname +"\n"
"Technically the member function '$symbol' can be static (but you may consider moving to unnamed namespace).\n"
"The member function '$symbol' can be made a static "
+ msg +
"function. Making a function static can bring a performance benefit since no 'this' instance is "
"passed to the function. This change should not cause compiler errors but it does not "
"necessarily make sense conceptually. Think about your design and the task of the function first - "
"is it a function that must not access members of class instances? And maybe it is more appropriate "
"to move this function to an unnamed namespace.", CWE398, Certainty::inconclusive);
}
}

//---------------------------------------------------------------------------
Expand Down
4 changes: 2 additions & 2 deletions lib/checkclass.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,8 @@ class CPPCHECKLIB CheckClass : public Check {
void operatorEqShouldBeLeftUnimplementedError(const Token *tok);
void operatorEqMissingReturnStatementError(const Token *tok, bool error);
void operatorEqToSelfError(const Token *tok);
void checkConstError(const Token *tok, const std::string &classname, const std::string &funcname, bool suggestStatic);
void checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname, bool suggestStatic);
void checkConstError(const Token *tok, const std::string &classname, const std::string &funcname, bool suggestStatic, bool foundAllBaseClasses = true);
void checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname, bool suggestStatic, bool foundAllBaseClasses = true);
void initializerListError(const Token *tok1,const Token *tok2, const std::string & classname, const std::string &varname, const std::string& argname = {});
void suggestInitializationList(const Token *tok, const std::string& varname);
void selfInitializationError(const Token* tok, const std::string& varname);
Expand Down
4 changes: 3 additions & 1 deletion lib/symboldatabase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4574,13 +4574,15 @@ void Function::addArguments(const SymbolDatabase *symbolDatabase, const Scope *s
}
}

bool Function::isImplicitlyVirtual(bool defaultVal) const
bool Function::isImplicitlyVirtual(bool defaultVal, bool* pFoundAllBaseClasses) const
{
if (hasVirtualSpecifier() || hasOverrideSpecifier() || hasFinalSpecifier())
return true;
bool foundAllBaseClasses = true;
if (getOverriddenFunction(&foundAllBaseClasses)) //If it overrides a base class's method then it's virtual
return true;
if (pFoundAllBaseClasses)
*pFoundAllBaseClasses = foundAllBaseClasses;
if (foundAllBaseClasses) //If we've seen all the base classes and none of the above were true then it must not be virtual
return false;
return defaultVal; //If we can't see all the bases classes then we can't say conclusively
Expand Down
2 changes: 1 addition & 1 deletion lib/symboldatabase.h
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,7 @@ class CPPCHECKLIB Function {
void addArguments(const SymbolDatabase *symbolDatabase, const Scope *scope);

/** @brief check if this function is virtual in the base classes */
bool isImplicitlyVirtual(bool defaultVal = false) const;
bool isImplicitlyVirtual(bool defaultVal = false, bool* pFoundAllBaseClasses = nullptr) const;

std::vector<const Function*> getOverloadedFunctions() const;

Expand Down
2 changes: 2 additions & 0 deletions test/cfg/qt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,7 @@ namespace {
class Fred : public QObject {
Q_OBJECT
private slots:
// cppcheck-suppress functionStatic
void foo();
};
void Fred::foo() {}
Expand Down Expand Up @@ -726,6 +727,7 @@ namespace {
~MyObject1() {}
public slots:
signals:
// cppcheck-suppress functionStatic
void test() {}
};

Expand Down
17 changes: 13 additions & 4 deletions test/testclass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ class TestClass : public TestFixture {
TEST_CASE(const93);
TEST_CASE(const94);
TEST_CASE(const95); // #13320 - do not warn about r-value ref method
TEST_CASE(const96);

TEST_CASE(const_handleDefaultParameters);
TEST_CASE(const_passThisToMemberOfOtherClass);
Expand Down Expand Up @@ -5041,7 +5042,7 @@ class TestClass : public TestFixture {
"public:\n"
" void f(){}\n"
"};");
ASSERT_EQUALS("", errout_str());
ASSERT_EQUALS("[test.cpp:3]: (performance, inconclusive) Either there is a missing 'override', or the member function 'derived::f' can be static.\n", errout_str());
}

void const34() { // ticket #1964
Expand Down Expand Up @@ -5827,7 +5828,8 @@ class TestClass : public TestFixture {
" inherited::set(inherited::Key(key));\n"
" }\n"
"};\n", nullptr, false);
ASSERT_EQUALS("", errout_str());
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2]: (performance, inconclusive) Either there is a missing 'override', or the member function 'MixerParticipant::GetAudioFrame' can be static.\n",
errout_str());
}

void const62() {
Expand Down Expand Up @@ -6674,8 +6676,6 @@ class TestClass : public TestFixture {
"struct S<0> {};\n"
"struct D : S<150> {};\n");
// don't hang
// we are not interested in the output - just consume it
ignore_errout();
}

void const93() { // #12162
Expand Down Expand Up @@ -6714,6 +6714,15 @@ class TestClass : public TestFixture {
ASSERT_EQUALS("", errout_str());
}

void const96() { // #13282
checkConst("struct S : B {\n"
" bool f() { return b; }\n"
" bool g() override { return b; }\n"
" bool b;\n"
"};\n");
ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Either there is a missing 'override', or the member function 'S::f' can be const.\n", errout_str());
}

void const_handleDefaultParameters() {
checkConst("struct Foo {\n"
" void foo1(int i, int j = 0) {\n"
Expand Down
Loading