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

Enhance Efficiency, Readability, and Performance in Various Cppcheck Modules #6650

Closed
wants to merge 1 commit into from

Conversation

merttozer
Copy link

This pull request includes a series of improvements aimed at enhancing the efficiency, readability, and performance of several key modules in the Cppcheck project. The changes made are detailed as follows:

  • executor.cpp:

    • Used initializer lists in the constructor
    • Simplified assertion and used std::lock_guard for mutex
  • singleexecutor.cpp and singleexecutor.h:

    • Simplified constructor initialization
    • Used range-based for loops and std::accumulate with lambda for better readability and efficiency
  • aboutdialog.cpp and aboutdialog.h:

    • Replaced raw pointers with std::unique_ptr
    • Simplified string formatting and used modern C++ features
    • Improved memory management and added const correctness
  • applicationdialog.cpp and applicationdialog.h:

    • Replaced raw pointers with std::unique_ptr
    • Improved memory management and added const correctness

- executor.cpp:
  - Used initializer lists in the constructor
  - Simplified assertion and used std::lock_guard for mutex

- singleexecutor.cpp and singleexecutor.h:
  - Simplified constructor initialization
  - Used range-based for loops and std::accumulate with lambda for better readability and efficiency

- aboutdialog.cpp and aboutdialog.h:
  - Replaced raw pointers with std::unique_ptr
  - Simplified string formatting and used modern C++ features
  - Improved memory management and added const correctness

- applicationdialog.cpp and applicationdialog.h:
  - Replaced raw pointers with std::unique_ptr
  - Improved memory management and added const correctness
@firewave
Copy link
Collaborator

Thanks for your contribution.

The PR appears to be incomplete though as some changes only have been partially applied and the description does not match the actual changes.

Also for various reasons it would be better to group the various changes into separate PRs.

Also if you claim performance improvement please provide some actual proof.

@@ -35,7 +35,7 @@ ApplicationDialog::ApplicationDialog(const QString &title,
Application &app,
QWidget *parent) :
QDialog(parent),
mUI(new Ui::ApplicationDialog),
mUI(std::make_unique<Ui::ApplicationDialog>()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

std::make_unique is not available until C++14 and the code targets C++11.

@@ -49,9 +49,9 @@ unsigned int SingleExecutor::check()
std::size_t processedsize = 0;
unsigned int c = 0;

for (std::list<FileWithDetails>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have not changed such loops yet since they are in a mutable scope and you cannot fully enforce the intent of constness in a range loop until C++17 which introduces std::as_const (I have been trying to add a helper for that).

That seems pedantic but since changing the code does not actually change anything it doesn't matter which style you use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Coincidentally I finally ran into some code which highlights the issue - see #6653.

@firewave
Copy link
Collaborator

No reply in over a month. Closing.

@firewave firewave closed this Aug 31, 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.

2 participants