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

Cppcheck and clazy warning fixes #2821

Merged
merged 6 commits into from
Jan 29, 2023
Merged

Cppcheck and clazy warning fixes #2821

merged 6 commits into from
Jan 29, 2023

Conversation

ElTh0r0
Copy link
Contributor

@ElTh0r0 ElTh0r0 commented Jul 23, 2022

I hope such kind of code quality improvements are wanted, since I'm touching many files with just "minor" changes.
This patch is mainly fixing warnings related to Cppcheck & Clazy passing args-by-ref / args-by-value, old style Qt signal/slots and using variable names same to function names.

@mmahmoudian
Copy link
Member

Thanks for the PR. I think this should be discussed. some of the changes seems reasonable to me, but some not so much. For instance:

imho good change:

  • void addToNameEditor(QString s); -> void addToNameEditor(const QString& s);

imho not so good change:

  • const QString& value = *it; -> const QString& val = *it;

@mmahmoudian mmahmoudian added the Needs Decision This is something that should be discussed by community label Dec 16, 2022
@ElTh0r0
Copy link
Contributor Author

ElTh0r0 commented Dec 16, 2022

imho not so good change:

* `const QString& value = *it;` -> `const QString& val = *it;`

reason for above change is that there is a function with name "value". So either the variable or the function name should be changed:

QString CommandLineParser::value(const CommandOption& option) const

I did some more variable renaming due to the same reason in other files.

Copy link
Contributor

@veracioux veracioux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ElTh0r0 Thanks for contributing to Flameshot. Some of them you should revert - I left comments to let you know which. But I very much appreciate you changing the SIGNAL(...) and SLOT(...) syntax. This was something I was going to do but never got around to it.

src/config/cacheutils.cpp Outdated Show resolved Hide resolved
src/core/qguiappcurrentscreen.cpp Outdated Show resolved Hide resolved
src/utils/confighandler.cpp Outdated Show resolved Hide resolved
src/utils/screengrabber.cpp Outdated Show resolved Hide resolved
src/widgets/orientablepushbutton.cpp Show resolved Hide resolved
@mmahmoudian mmahmoudian removed the Needs Decision This is something that should be discussed by community label Jan 21, 2023
@ElTh0r0
Copy link
Contributor Author

ElTh0r0 commented Jan 21, 2023

@veracioux many thanks for the detailed review! Please check my changes after the last commits.

Copy link
Contributor

@veracioux veracioux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ElTh0r0 Thanks. All looks good now.

@mmahmoudian
Copy link
Member

@veracioux should we proceed with the merge?

@veracioux veracioux merged commit f7e41f4 into flameshot-org:master Jan 29, 2023
@ElTh0r0 ElTh0r0 deleted the cppcheck-fixes branch February 2, 2023 19:27
panpuchkov pushed a commit to namecheap/flameshot that referenced this pull request Feb 18, 2023
* Cppcheck and clazy warning fixes

* Clang-format

* Revert QRect call by value

* Revert QPoint call by value

* Revert complained renamings

---------

Co-authored-by: Haris Gušić <[email protected]>
(cherry picked from commit f7e41f4)
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