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

pass strings by const reference and use find_last_not_of #779

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

soyersoyer
Copy link
Contributor

@soyersoyer soyersoyer commented Dec 30, 2024

const reference opportunities were advised by cppcheck to skip copy construction

Copy link

Build for testing:
MiniDexed_2024-12-30-37ce0ee
Use at your own risk.

@probonopd
Copy link
Owner

If I understand it right:

  • It improves the efficiency of string handling by using const std::string & instead of std::string as function parameters. This avoids unnecessary string copies and reduces memory allocation.
  • It simplifies the code in CPerformanceConfig::SetNewPerformanceName by using the find_last_not_of method to remove trailing spaces from the input string. This replaces the original loop that iterated over the string to find the last non-space character.
  • The changes are consistent across multiple files (minidexed.cpp, minidexed.h, performanceconfig.cpp, and performanceconfig.h), ensuring that the improvements are applied uniformly throughout the codebase.

Thanks @soyersoyer. Were you able to test it thoroughly so it's low risk to merge?

@soyersoyer
Copy link
Contributor Author

Yes.

Copy link
Collaborator

@diyelectromusic diyelectromusic left a comment

Choose a reason for hiding this comment

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

Remember I'm not a C++ expert, but from what I think I know of references, I can't see any issues with this.

@probonopd probonopd merged commit 0f7f8f4 into probonopd:main Jan 2, 2025
1 check passed
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