Revert changes to transposeTpc#33325
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/engraving/editing/transpose.cpp (3)
732-734: 💤 Low valueMisleading guard comment, and consider using
tpcIsValidfor consistency.The comment
// perfect unison & perfect octavedoes not describe this guard:TPC_INVALIDis a general "no spelling" sentinel. The perfect-unison/zero-interval path is handled separately at lines 749–751. Additionally,transposeTpcDiatonicByKey(line 685) usestpcIsValid(tpc)for the same kind of guard; using the bare sentinel comparison here is inconsistent and would pass through any out-of-range TPC value that isn't exactlyTPC_INVALID.♻️ Proposed cleanup
- if (tpc == Tpc::TPC_INVALID) { // perfect unison & perfect octave + if (!tpcIsValid(tpc)) { return tpc; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engraving/editing/transpose.cpp` around lines 732 - 734, The guard comparing tpc to Tpc::TPC_INVALID is misleading and inconsistent with other code; replace the bare sentinel check with a call to tpcIsValid(tpc) (i.e., if (!tpcIsValid(tpc)) return tpc;) so any invalid/out-of-range TPC values are consistently handled, and change the trailing comment to describe that this is a general "invalid/no spelling" sentinel (not "perfect unison & perfect octave"); note that the perfect-unison path is handled elsewhere (as in transposeTpcDiatonicByKey) so keep that separate.
748-788: 💤 Low valueCommented-out dead code — consider cleaning up as part of this revert.
Four commented-out
LOGDstatements (lines 748, 786, 788) and an alternative code fragment (lines 765–770) are legacy artifacts that rode along with the revert. They add visual noise and make the loop harder to read.♻️ Proposed cleanup (remove commented-out lines)
-// LOGD("transposeTpc tpc %d steps %d semitones %d", tpc, steps, semitones); if (semitones == 0 && steps == 0) { return tpc; } ... alter = semitones - (p1 - pitch); - // alter = p1 + semitones - pitch; - -// if (alter < 0) { -// alter *= -1; -// alter = 12 - alter; -// } while (alter < 0) { ... -// LOGD(" again alter %d steps %d, step %d", alter, steps, step); } -// LOGD(" = step %d alter %d tpc %d", step, alter, step2tpc(step, alter)); return step2tpc(step, AccidentalVal(alter));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engraving/editing/transpose.cpp` around lines 748 - 788, The loop in transposeTpc contains commented-out debug LOGD calls and an inactive alternative implementation (the p1+semitones line and the commented block that flips alter) that are dead code and should be removed to improve readability; edit transposeTpc in transpose.cpp to delete the commented LOGD statements and the obsolete alternative fragment (including the commented if(alter < 0) block and the commented assignment alter = p1 + semitones - pitch) so only the active logic remains, leaving variable names (step, alter, pitch, p1) and the loop intact.
757-789: 💤 Low valueSilent fallback if the search loop does not converge.
If no valid
alteris found within 10 iterations the function falls through and returnsstep2tpc(step, AccidentalVal(alter))withalterstill outside[minAlter, maxAlter], producing an out-of-range TPC silently. For every valid musical interval convergence is guaranteed in ≤ 3 iterations, so this is not a reachable failure path in practice — but the removedLOGDcalls (now commented out) were the only diagnostic for detecting it. Adding a post-loop assertion or log would make pathological inputs visible during development.♻️ Proposed observability addition
-// LOGD(" again alter %d steps %d, step %d", alter, steps, step); } -// LOGD(" = step %d alter %d tpc %d", step, alter, step2tpc(step, alter)); + if (alter < minAlter || alter > maxAlter) { + LOGE("transposeTpc: failed to find valid alteration (tpc=%d, diatonic=%d, chromatic=%d)", + tpc, interval.diatonic, interval.chromatic); + } return step2tpc(step, AccidentalVal(alter));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engraving/editing/transpose.cpp` around lines 757 - 789, The loop in transpose.cpp that searches for a valid alter may exit after 10 iterations without meeting minAlter/maxAlter, then returns step2tpc(step, AccidentalVal(alter)) silently; add a post-loop check using the same symbols (step, alter, minAlter, maxAlter, steps) to detect non-convergence: if alter is outside [minAlter, maxAlter] emit a diagnostic (LOGD/LOGE) or assert with contextual info (step, steps, tpc, semitones, pitch) and then either clamp alter into the allowed range before calling step2tpc or fail fast, so pathological inputs are visible during development.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/engraving/editing/transpose.cpp`:
- Around line 732-734: The guard comparing tpc to Tpc::TPC_INVALID is misleading
and inconsistent with other code; replace the bare sentinel check with a call to
tpcIsValid(tpc) (i.e., if (!tpcIsValid(tpc)) return tpc;) so any
invalid/out-of-range TPC values are consistently handled, and change the
trailing comment to describe that this is a general "invalid/no spelling"
sentinel (not "perfect unison & perfect octave"); note that the perfect-unison
path is handled elsewhere (as in transposeTpcDiatonicByKey) so keep that
separate.
- Around line 748-788: The loop in transposeTpc contains commented-out debug
LOGD calls and an inactive alternative implementation (the p1+semitones line and
the commented block that flips alter) that are dead code and should be removed
to improve readability; edit transposeTpc in transpose.cpp to delete the
commented LOGD statements and the obsolete alternative fragment (including the
commented if(alter < 0) block and the commented assignment alter = p1 +
semitones - pitch) so only the active logic remains, leaving variable names
(step, alter, pitch, p1) and the loop intact.
- Around line 757-789: The loop in transpose.cpp that searches for a valid alter
may exit after 10 iterations without meeting minAlter/maxAlter, then returns
step2tpc(step, AccidentalVal(alter)) silently; add a post-loop check using the
same symbols (step, alter, minAlter, maxAlter, steps) to detect non-convergence:
if alter is outside [minAlter, maxAlter] emit a diagnostic (LOGD/LOGE) or assert
with contextual info (step, steps, tpc, semitones, pitch) and then either clamp
alter into the allowed range before calling step2tpc or fail fast, so
pathological inputs are visible during development.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df058d0e-1a96-4788-8801-aede76121794
📒 Files selected for processing (1)
src/engraving/editing/transpose.cpp
2 participants
Resolves: #33312
Reverts changes to
transposeTpcmade in #30139. The old implementation handles enharmonic transpositions, though not in the most clear way.