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

[Mu4] fix #327681 illegal repeatsegment crash #10159

Merged
merged 2 commits into from
Sep 26, 2022

Conversation

jeetee
Copy link
Contributor

@jeetee jeetee commented Dec 22, 2021

illegal repeatsegment inserted due to a certain open volta, jump and end repeat combination

Resolves: https://musescore.org/en/node/327681

MU4 version of PR #10158
Fixes interpretation order so the end repeat is covered by the open volta if a jump also exists in the same measure.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • [N/A - patch applied according to what builds in Mu3] I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@jeetee
Copy link
Contributor Author

jeetee commented Dec 23, 2021

We might want to update Q_ASSERTS to IF_ASSERT_FAILED as per https://github.com/musescore/MuseScore/wiki/Error-handling; but I've not included it here yet, as it would touch more in this file than relevant to the issue.
Perhaps that replacement could be done in a code sweep commit?

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Sep 24, 2022

Rebase needed (merge conflict must be in the testing part). Issue came up again in https://musescore.org/en/node/335346

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Sep 26, 2022

no Q_ASSERT() without Qt...
So see your own comment at #issuecomment-1000158786

Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz left a comment

Choose a reason for hiding this comment

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

While I don't claim to fully understand it all, the proposed change does make sense to me ;-)
Also, except for the now removed Q_ASSERT() it is the same code as what I 'stole' for #9000 (rather: picked from #10158), and there it does work.

@jeetee
Copy link
Contributor Author

jeetee commented Sep 26, 2022

@cbjeukendrup would be nice to have this merged in before beta; one less crash to worry about then ;-)

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Sep 26, 2022

It looks good to me, so I've asked @vpereverzev to take a look too, since we're already entering extra careful mode :)

When it comes to asserts, we have two flavours:

  • simple (C++ built-in?) assert(…)
  • the IF_ASSERT_FAILED macro:
    IF_ASSERT_FAILED(condition) {
        // code that will be run when condition is unexpectedly false
        // (if asserts are enabled (debug build), then the assert would trigger an abort and this point won't be reached)
    }

@jeetee
Copy link
Contributor Author

jeetee commented Sep 26, 2022

The Q_ASSERT was there more as a glorified debug breakpoint; the further handling of the scenario is the same whether the assert would've been hit or not.

@vpereverzev vpereverzev merged commit 294b8d8 into musescore:master Sep 26, 2022
@jeetee
Copy link
Contributor Author

jeetee commented Sep 26, 2022

ty!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants