Skip to content

Engraving: add missing reserve()#33477

Open
RomanPudashkin wants to merge 1 commit into
musescore:mainfrom
RomanPudashkin:engraving_add_reserve
Open

Engraving: add missing reserve()#33477
RomanPudashkin wants to merge 1 commit into
musescore:mainfrom
RomanPudashkin:engraving_add_reserve

Conversation

@RomanPudashkin
Copy link
Copy Markdown
Contributor

No description provided.

@RomanPudashkin RomanPudashkin requested a review from mike-spa May 19, 2026 12:45
@RomanPudashkin RomanPudashkin added performance Performance issues (e.g. high CPU usage) tech debt labels May 19, 2026
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b0127230-dd0d-48e7-9356-89dff7c9688d

📥 Commits

Reviewing files that changed from the base of the PR and between 00d8cc8 and aaeb6d5.

📒 Files selected for processing (22)
  • src/engraving/dom/beam.cpp
  • src/engraving/dom/beambase.cpp
  • src/engraving/dom/chord.cpp
  • src/engraving/dom/excerpt.cpp
  • src/engraving/dom/figuredbass.cpp
  • src/engraving/dom/glissando.cpp
  • src/engraving/dom/instrument.cpp
  • src/engraving/dom/measure.cpp
  • src/engraving/dom/range.cpp
  • src/engraving/dom/repeatlist.cpp
  • src/engraving/dom/score.cpp
  • src/engraving/dom/scoreorder.cpp
  • src/engraving/dom/staff.cpp
  • src/engraving/dom/stafftype.cpp
  • src/engraving/dom/stringdata.cpp
  • src/engraving/dom/unrollrepeats.cpp
  • src/engraving/editing/editchord.cpp
  • src/engraving/editing/transpose.cpp
  • src/engraving/rendering/score/boxlayout.cpp
  • src/engraving/rendering/score/scorehorizontalviewlayout.cpp
  • src/engraving/rendering/score/tremololayout.cpp
  • src/engraving/types/typesconv.cpp
✅ Files skipped from review due to trivial changes (17)
  • src/engraving/dom/staff.cpp
  • src/engraving/dom/stafftype.cpp
  • src/engraving/rendering/score/scorehorizontalviewlayout.cpp
  • src/engraving/dom/chord.cpp
  • src/engraving/rendering/score/boxlayout.cpp
  • src/engraving/dom/range.cpp
  • src/engraving/dom/excerpt.cpp
  • src/engraving/editing/editchord.cpp
  • src/engraving/dom/score.cpp
  • src/engraving/dom/unrollrepeats.cpp
  • src/engraving/dom/measure.cpp
  • src/engraving/dom/beam.cpp
  • src/engraving/dom/instrument.cpp
  • src/engraving/types/typesconv.cpp
  • src/engraving/dom/scoreorder.cpp
  • src/engraving/dom/beambase.cpp
  • src/engraving/dom/repeatlist.cpp

📝 Walkthrough

Walkthrough

This pull request applies vector capacity preallocation optimizations across MuseScore's codebase. Multiple functions now call reserve() on vectors before populating them, reducing dynamic reallocations during growth. Changes span beam structures, chord data, core domain objects, specialized DOM elements, editing operations, rendering/layout code, and type conversion routines. One function adds bounds clamping. No public APIs, logic, or control flow are altered—only memory allocation patterns are optimized.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request has no description provided by the author, missing required template sections including motivation, issue resolution, and verification checkboxes. Add a description explaining the performance motivation, reference the related issue, and complete the verification checklist from the template.
Docstring Coverage ⚠️ Warning Docstring coverage is 41.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding reserve() calls to preallocate vector capacity across multiple engraving DOM and rendering files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@src/engraving/dom/stringdata.cpp`:
- Around line 52-53: Guard the negative numStrings before calling
m_stringTable.reserve to avoid signed-to-size_t wraparound: check if numStrings
> 0 and only then call m_stringTable.reserve(static_cast<size_t>(numStrings));
likewise ensure the subsequent for (int i = 0; i < numStrings; i++) loop is
skipped when numStrings <= 0 (or change loop to use a size_t count variable
derived from the validated numStrings) so no huge allocation or invalid
iteration can occur.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 567cf179-412f-4060-a49d-fb553843f175

📥 Commits

Reviewing files that changed from the base of the PR and between 283bb5b and 64ddd1f.

📒 Files selected for processing (23)
  • muse
  • src/engraving/dom/beam.cpp
  • src/engraving/dom/beambase.cpp
  • src/engraving/dom/chord.cpp
  • src/engraving/dom/excerpt.cpp
  • src/engraving/dom/figuredbass.cpp
  • src/engraving/dom/glissando.cpp
  • src/engraving/dom/instrument.cpp
  • src/engraving/dom/measure.cpp
  • src/engraving/dom/range.cpp
  • src/engraving/dom/repeatlist.cpp
  • src/engraving/dom/score.cpp
  • src/engraving/dom/scoreorder.cpp
  • src/engraving/dom/staff.cpp
  • src/engraving/dom/stafftype.cpp
  • src/engraving/dom/stringdata.cpp
  • src/engraving/dom/unrollrepeats.cpp
  • src/engraving/editing/editchord.cpp
  • src/engraving/editing/transpose.cpp
  • src/engraving/rendering/score/boxlayout.cpp
  • src/engraving/rendering/score/scorehorizontalviewlayout.cpp
  • src/engraving/rendering/score/tremololayout.cpp
  • src/engraving/types/typesconv.cpp

Comment thread src/engraving/dom/stringdata.cpp Outdated
@RomanPudashkin RomanPudashkin force-pushed the engraving_add_reserve branch 2 times, most recently from dc5d702 to 00d8cc8 Compare May 21, 2026 10:30
@RomanPudashkin RomanPudashkin force-pushed the engraving_add_reserve branch from 00d8cc8 to aaeb6d5 Compare May 21, 2026 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Performance issues (e.g. high CPU usage) tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant