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

BREAKING: Static field cleanup, #662, #679 #1079

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

Conversation

paulirwin
Copy link
Contributor

@paulirwin paulirwin commented Dec 31, 2024

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Converts static readonly fields to const, where possible. Also addresses csharpsquid:S2223 issues with mutable static fields.

Fixes #662
Fixes #679

Description

This PR converts most static readonly fields to const where it is possible to do so. Some notable exceptions:

  1. The analysis was not focused on any fields in unit test projects.
  2. Any fields whose values changed in the latest Lucene were kept as static readonly, to be on the safe side.
  3. If there were other, similar fields that needed to be static readonly, the ones that were const-able were left as-is for consistency.
  4. Fields like VERSION_CURRENT were left as-is, since it might be expected to evaluate those.

This also addresses #679 with non-readonly static fields, where one of three things happened:

  1. If the visibility should be private, this visibility was changed to private.
  2. If the field really should have been readonly or const, this was changed appropriately.
  3. If the field really should be mutable, it was changed into a static property.

@paulirwin paulirwin added the is:enhancement New feature or request label Dec 31, 2024
@paulirwin paulirwin added notes:breaking-change Has changes that will break backward compatibility and removed is:enhancement New feature or request labels Dec 31, 2024
@paulirwin paulirwin changed the title Convert static readonly fields to const, #662 BREAKING: Convert static readonly fields to const, #662 Dec 31, 2024
@paulirwin paulirwin marked this pull request as draft January 1, 2025 21:17
@paulirwin
Copy link
Contributor Author

paulirwin commented Jan 1, 2025

Converted to draft; I'm going to include the related #679 changes as well.

Update: this is complete.

@paulirwin paulirwin changed the title BREAKING: Convert static readonly fields to const, #662 BREAKING: Static field cleanup, #662, #679 Jan 1, 2025
@paulirwin paulirwin marked this pull request as ready for review January 1, 2025 22:45
@paulirwin
Copy link
Contributor Author

@NightOwl888 I'm iffy on ExtractWikipedia.Count. It's clearly internally mutable, and in Lucene it's possible to mutate it externally, but I don't think it's really supposed to be externally mutable (or even externally readable for that matter). It's pretty low priority to fix this, as this is just in benchmark code so not likely used in the real world, but let me know what you would do here. I could either make it have a private setter, or make it just a private static field instead of public. I also didn't change the increment to use Interlocked since this is not likely to be called so often in a multi-threaded way to where that would be an issue, but I could.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:breaking-change Has changes that will break backward compatibility
Projects
None yet
1 participant