Skip to content

Instantiate mutexes by value #4632

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Instantiate mutexes by value #4632

wants to merge 3 commits into from

Conversation

stefanhaller
Copy link
Collaborator

  • PR Description

Here's a minor cleanup: instantiate mutexes by value so that they don't have to be initialized explicitly. It is always preferable for structs to have a valid zero value, and this is one small step in that direction.

I read this recommendation in the Uber style guide, and it makes a lot of sense to me. I find most of the rest of this style guide to be a very good read, too.

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@stefanhaller stefanhaller added the maintenance For refactorings, CI changes, tests, version bumping, etc label Jun 9, 2025
Copy link

codacy-production bot commented Jun 9, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for fb2c5ea1 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (fb2c5ea) Report Missing Report Missing Report Missing
Head commit (53d195f) 56671 49222 86.86%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4632) 5 5 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

Comment on lines 316 to 317
// if you add a new mutex here be sure to instantiate it. We're using pointers to
// mutexes so that we can pass the mutexes to controllers.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems to now be out of date, does its implications still apply?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course, that was the whole point. Removed in 53d195f.

Instead, pass the entire Mutexes struct by pointer to controllers.

This is better because Mutexes now has a zero value and doesn't need to be
initialized.
Like in the previous commit, this is preferred because the fields don't need to
be initialized this way.
Copy link
Contributor

@ChrisMcD1 ChrisMcD1 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance For refactorings, CI changes, tests, version bumping, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants