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 two types of Clazy warnings #5807

Merged
merged 2 commits into from
Dec 27, 2023

Conversation

gruenich
Copy link
Contributor

Chained QString::arg, replace inclusion of QtCore

This saves memory allocations.
Found by Clazy (qstring-arg).
Only include classes that are actually used.
Do not pull in all internal dependencies.
Found by Clazy (no-module-include).
@chrchr-github chrchr-github merged commit 4a9b921 into danmar:main Dec 27, 2023
68 checks passed
@firewave
Copy link
Collaborator

firewave commented Jan 5, 2024

It appears multi-arg only works when the arguments are QString.

The GUI now reports error to the terminal (see https://trac.cppcheck.net/ticket/12316):

Could not parse stylesheet of object CodeEditor(0x7fffe6f7de60, name = "CodeEditor")

The code in question is in CodeEditor::generateStyleString():

    QString bgcolor = QString("background:rgb(%1,%2,%3);")
                      .arg(mWidgetStyle->widgetBGColor.red(),
                           mWidgetStyle->widgetBGColor.green(),
                           mWidgetStyle->widgetBGColor.blue());

All of the arguments are int and the called function appears to be QString::arg(int a, int fieldWidth, int base, QChar fillChar) according to my IDE. The actual multi-arg function I see in the header though is QString::arg(const QString &a1, const QString &a2, const QString &a3).

This is with Qt5 though. I have not checked if Qt6 has additional overloads.

@firewave
Copy link
Collaborator

firewave commented Jan 5, 2024

Qt6 has a varargs overload but that also only works with strings.

From https://doc.qt.io/qt-6/qstring.html#arg-14:

Args can consist of anything that implicitly converts to [QString](https://doc.qt.io/qt-6/qstring.html), [QStringView](https://doc.qt.io/qt-6/qstringview.html) or [QLatin1StringView](https://doc.qt.io/qt-6/qlatin1stringview.html).

In addition, the following types are also supported: [QChar](https://doc.qt.io/qt-6/qchar.html), [QLatin1Char](https://doc.qt.io/qt-6/qlatin1char.html).

@chrchr-github
Copy link
Collaborator

chrchr-github commented Jan 6, 2024

It sounds like the clazy check should detect exactly this kind of misuse that we have now:
https://github.com/KDE/clazy/blob/master/docs/checks/README-qstring-arg.md
Edit: from the bottom:
Using these misleading overloads is perfectly valid, so only warning (1) is enabled by default. To enable warning (2), export CLAZY_EXTRA_OPTIONS="qstring-arg-fillChar-overloads
Maybe we should run clazy in our CI?

@firewave
Copy link
Collaborator

firewave commented Jan 6, 2024

Maybe we should run clazy in our CI?

I thought about that but it doesn't offer that much of interest and has several false positives.

My IDE supports Clazy but it seems I am not able to specify the extra option.

@chrchr-github
Copy link
Collaborator

@gruenich Can you revert the arg() change?

@gruenich
Copy link
Contributor Author

gruenich commented Jan 7, 2024

@gruenich Can you revert the arg() change?

I'd prefer a partial revert to get it running on Ubuntu 20.04, too. Focal provides an older version of Qt 5.12 which might lack some features.
To identify the breaking part of the changes, I compiled the tip of main on a Ubuntu Focal Docker. It just compiled without an error, so I have no indication which parts of my change are good and which are not.
Further, running Clazy with the extra checks (CLAZY_EXTRA_OPTIONS="qstring-arg-fillChar-overloads") did not help neither.

firewave added a commit to firewave/cppcheck that referenced this pull request Jan 20, 2024
@firewave
Copy link
Collaborator

Reverted in #5902.

chrchr-github pushed a commit that referenced this pull request Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants