Add helper methods to ViewingRules class - #2330
Open
hmms wants to merge 1 commit into
Open
Conversation
Hoists viewingRulesAreEqual and copyViewingRule out of a private, file-local anonymous namespace in apphelpers/mergeconfigs/SectionMerger.cpp (added by the merge-configs feature, PR AcademySoftwareFoundation#2179) into ViewingRules.h/.cpp as reusable helpers, matching the existing free-function pattern already used there for FindRule. Both functions already matched the exact signatures given in AcademySoftwareFoundation#2050, so this is a pure relocation: names and bodies are unchanged, and none of SectionMerger.cpp's call sites needed to change. Adds direct unit tests for both functions to ViewingRules_tests.cpp, which previously only had indirect coverage via the MergeConfigs test group. Also: removes the now-unused TokensManager.h include from SectionMerger.cpp (its only prior use was inside the hoisted viewingRulesAreEqual), documents that copyViewingRule's src/rules arguments must not alias each other, and adds a test case for custom-key name (not just value) mismatches. Fixes AcademySoftwareFoundation#2050 Signed-off-by: Muralidhar M Shenoy <shenoy.muralidhar.m@gmail.com>
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
Hoists
viewingRulesAreEqualandcopyViewingRuleout ofSectionMerger.cpp's anonymous namespace and ontoViewingRulesas reusable free functions (ViewingRules.h/ViewingRules.cpp), matching the existingFindRulepattern in that file. Adds direct unit tests for both, per the issue's explicit "unit tests must be added" ask.Closes #2050
Background
Both functions already existed, byte-for-byte matching this issue's own suggested signatures, but as private, file-local helpers added later by #2179 ("Adsk Contrib - Merge configs feature"), apparently written independently to solve the same internal need rather than in response to this issue. They're used at 7 call sites in
SectionMerger.cpp's config-merge logic but were never exposed as a reusable, directly-testableViewingRuleshelper. (A sibling function,fileRulesAreEqual, is equally un-hoisted, so this isn't fixing an inconsistency, just closing this specific gap.)Implementation
Function bodies and names are moved verbatim, no logic changes, so none of the 7 existing call sites in
SectionMerger.cppneeded to change (just delete the two definitions there and add#include "ViewingRules.h").git diff --statshows +239/-113, but 112 of the added lines and all 113 removed lines are the moved functions; the only genuinely new content is ~17 lines of header declarations/doc comments and ~115 lines of new tests.Two things worth flagging rather than silently deciding myself:
viewingRulesAreEqual/copyViewingRuleare camelCase, inherited verbatim from the issue and the pre-existing code, against this file's PascalCase convention for other free functions (FindRule,IsEncodingUsed). Left as-is rather than renaming out from under the issue's own spec, but happy to rename if you'd prefer consistency with the rest of the file.copyViewingRule's doc comment now notes a pre-existing (not introduced here) aliasing edge case: ifsrcandrulesare the same object anddstIdx < srcIdx, the copy can leave a spurious empty rule behind rather than throwing. None of the 7 real call sites do this, so I documented it rather than adding defensive complexity for a case nothing currently reaches.Test plan
viewing_rules_are_equalandcopy_viewing_ruletotests/cpu/ViewingRules_tests.cpp, covering order-independence, count/content mismatches (colorspaces, encodings, and custom-key names and values), name-not-compared, multi-rule-set indexing, append-without-disturbing-existing-entries, and the no-op-on-duplicate-name behavior.MergeConfigstests (the existing caller of these functions, e.g.displays_views_section), confirming the hoist is behavior-preserving with no regression to config merging.upstream/main, no drift since this branch was cut (still at the same commit), issue still open/unassigned/uncommented, rebuilt and reran the full suite once more with the same result.Notes for maintainers
This is my second PR to OpenColorIO (alongside #2326, still awaiting review) and my first time working in this specific corner of the codebase, so please don't hesitate to push back on any of the judgment calls above (naming, scope, how much to document vs. fix) or point me at conventions I might have missed — happy to iterate.