Region/Type: add value-equals; DisplayLocation: suppress identity-equals#7638
Merged
Conversation
Fixes 3 EQ_COMPARETO_USE_OBJECT_EQUALS and 1 SI_INSTANCE_BEFORE_FINALS_ASSIGNED finding from SpotBugs. Region: add value-based equals/hashCode matching the case-insensitive compareTo. Region is used as a HashMap<Optional<Region>, ...> key in BioSet (ageMap, userMap) and .equals() is called directly in RegionFacet.matchesRegion. The intern map in getConstant() already made two getConstant(name) calls return the same instance, so the new equals/hashCode strengthens but does not change behaviour for any existing call site. Use String.CASE_INSENSITIVE_ORDER.compare(...) == 0 in equals to satisfy the find-sec-bugs IMPROPER_UNICODE rule (PR PCGen#7626 convention); hashCode folds via toLowerCase(Locale.ROOT) for a stable, locale-independent hash consistent with equals. Region: also rearrange the private static int ordinalCount = 0 above the NONE constant declaration to silence SI_INSTANCE_BEFORE_FINALS_ASSIGNED. JLS default-initialises primitive int to 0 before any field initialiser runs, so this is purely a static-layout cleanup. Type: add value-based equals/hashCode matching the case-sensitive compareTo. Type is used as HashSet<Type>/HashMap<Type, ...> across 8 production files. Since the intern map is CaseInsensitiveMap, two distinct Type instances with fieldNames differing only in case cannot exist, so case-sensitive equals matches compareTo and preserves existing behaviour. DisplayLocation: identity-equals is already correct (singleton-by- construction via getConstant intern map, no .equals() callers, not used as a collection key). Method-level @SuppressFBWarnings on compareTo documents that explicitly.
The original commit replaced the stale 'no equals/hashCode needed because TypeSafeConstant has a private constructor' comment with the actual equals/hashCode methods, but didn't leave anything in its place. Replaces the lost archival value with a short class-level Javadoc note explaining the design intent: equals/hashCode match compareTo (Comparable contract) and support use as a HashMap key (BioSet, RegionFacet, DataSet, GameMode, etc.). Captures the *why*, not the *what*, so future readers don't have to rediscover the contract reasoning from git history.
Vest
force-pushed
the
spotbugs-comparable-equals
branch
from
July 22, 2026 12:04
7a8bcf5 to
7052340
Compare
Vest
marked this pull request as ready for review
July 22, 2026 12:04
🚧 PCGen Code Coverage
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 3 EQ_COMPARETO_USE_OBJECT_EQUALS and 1 SI_INSTANCE_BEFORE_FINALS_ASSIGNED findings from SpotBugs in
pcgen.cdom.enumeration. All three classes (Region,Type,DisplayLocation) implementComparablewithout overridingequals/hashCode. Two of them are used as collection keys in production code; the third is not. Fix per case.Changes
Region.java (used as collection key)
equalsandhashCodematching the existing case-insensitivecompareTo.equalsusesString.CASE_INSENSITIVE_ORDER.compare(...) == 0(per PR IMPROPER_UNICODE: replace equalsIgnoreCase where safe, suppress where idiomatic #7626 convention) so the find-sec-bugs IMPROPER_UNICODE rule stays clean.hashCodefolds viatoLowerCase(Locale.ROOT)for a stable, locale-independent hash consistent with equals.private static int ordinalCount = 0above theNONEdeclaration to silenceSI_INSTANCE_BEFORE_FINALS_ASSIGNED(JLS default-initialisesintto 0 anyway; pure static-layout cleanup).Why safe:
Regionis used asOptional<Region>key inBioSet.ageMap/BioSet.userMap(DoubleKeyMap/TripleKeyMapToList) and.equals()is called directly inRegionFacet.matchesRegion. ThegetConstantintern map already made twogetConstant(name)calls return the same instance, so the new equals/hashCode strengthens but does not change behaviour for any existing call site.Type.java (used as collection key)
equalsandhashCodematching the existing case-sensitivecompareTo.Typeis used asHashSet<Type>/HashMap<Type, ...>in 8 production files.TYPE_MAPis aCaseInsensitiveMap, two distinctTypeinstances with fieldNames differing only in case cannot exist, so case-sensitive equals correctly matches compareTo.DisplayLocation.java (not used as collection key)
@SuppressFBWarnings("EQ_COMPARETO_USE_OBJECT_EQUALS")oncompareTo.getConstantintern map; no.equals()callers, not used as a collection key. Identity equality is correct here.Verification
./gradlew compileJava: BUILD SUCCESSFULpcgen.cdom.enumeration.*,pcgen.cdom.facet.*,pcgen.cdom.facet.fact.*,pcgen.core.*): 2528 tests, 0 failures, 0 errors.<BugInstance>rows):