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

Refactor: Pass "CommandLineOptions" to CSettings::Load by reference #3115

Merged

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Jul 29, 2023

Short description of changes

Pass "CommandLineOptions" to CSettings::Load by reference

CHANGELOG: SKIP

Context: Fixes an issue?

N/A

Does this change need documentation? What needs to be documented and how?

No

Status of this Pull Request

I was using it in my main build but I split it out to make it easier to raise the PR. I'll use it in my (GUI) client and (headless) server for today.

What is missing until this pull request can be merged?

Review.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@pljones pljones self-assigned this Jul 29, 2023
@pljones pljones added the refactoring Non-behavioural changes, Code cleanup label Jul 29, 2023
@pljones pljones added this to the Release 3.11.0 milestone Jul 29, 2023
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

We should check that this doesn't cause side effects. In the code later down (needs code reading).

@pljones
Copy link
Collaborator Author

pljones commented Jul 29, 2023

It shouldn't have any effect: the value is (well, should be) used only read-only in the code.

@pljones pljones force-pushed the options/pass-commandline-by-reference branch 3 times, most recently from 063809e to f64fa3d Compare August 3, 2023 18:35
@pljones pljones force-pushed the options/pass-commandline-by-reference branch from f64fa3d to 9c24c95 Compare August 15, 2023 16:16
@pljones pljones force-pushed the options/pass-commandline-by-reference branch 4 times, most recently from c3ae148 to d973663 Compare August 28, 2023 12:31
@pljones pljones force-pushed the options/pass-commandline-by-reference branch 2 times, most recently from ec9fc24 to fb11126 Compare September 12, 2023 16:20
@pljones pljones force-pushed the options/pass-commandline-by-reference branch from fb11126 to 2c461e2 Compare September 18, 2023 18:03
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Looking at the code, we do just use it with .contains, so everything is fine - as it should be.

@pljones pljones force-pushed the options/pass-commandline-by-reference branch from 2c461e2 to 18e235e Compare September 27, 2023 08:23
@pljones pljones merged commit a9b5475 into jamulussoftware:main Sep 27, 2023
9 checks passed
@pljones pljones deleted the options/pass-commandline-by-reference branch September 27, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Non-behavioural changes, Code cleanup
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants