From 8a94fb36818b7af8eb6fa5815c4836f19e7c2210 Mon Sep 17 00:00:00 2001 From: Dalton Messmer <33463986+messmerd@users.noreply.github.com> Date: Thu, 31 Aug 2023 12:55:02 -0400 Subject: [PATCH] Fix std::vector refactor mistake (#6836) * Use std::vector const reference instead of copying * Add assertions * Simplification --- include/MidiClip.h | 2 +- include/PianoRoll.h | 4 ++-- src/core/EffectChain.cpp | 3 +++ src/core/Mixer.cpp | 4 ++-- src/core/PatternStore.cpp | 12 +++++------ src/core/RenderManager.cpp | 4 ++-- src/core/Song.cpp | 2 +- src/gui/MixerView.cpp | 4 ++-- src/gui/clips/ClipView.cpp | 4 +++- src/gui/editors/PatternEditor.cpp | 4 ++-- src/gui/editors/PianoRoll.cpp | 29 ++++++++++++--------------- src/gui/tracks/TrackContentWidget.cpp | 4 ++-- src/tracks/MidiClip.cpp | 4 ++-- 13 files changed, 41 insertions(+), 39 deletions(-) diff --git a/include/MidiClip.h b/include/MidiClip.h index 43b322f8075..c2287bd009e 100644 --- a/include/MidiClip.h +++ b/include/MidiClip.h @@ -79,7 +79,7 @@ class LMMS_EXPORT MidiClip : public Clip void setStep( int step, bool enabled ); // Split the list of notes on the given position - void splitNotes(NoteVector notes, TimePos pos); + void splitNotes(const NoteVector& notes, TimePos pos); // clip-type stuff inline Type type() const diff --git a/include/PianoRoll.h b/include/PianoRoll.h index 9f3bbcd7d20..38788180f8f 100644 --- a/include/PianoRoll.h +++ b/include/PianoRoll.h @@ -308,9 +308,9 @@ protected slots: TimePos newNoteLen() const; void shiftPos(int amount); - void shiftPos(NoteVector notes, int amount); + void shiftPos(const NoteVector& notes, int amount); void shiftSemiTone(int amount); - void shiftSemiTone(NoteVector notes, int amount); + void shiftSemiTone(const NoteVector& notes, int amount); bool isSelection() const; int selectionCount() const; void testPlayNote( Note * n ); diff --git a/src/core/EffectChain.cpp b/src/core/EffectChain.cpp index b07a7227bd9..4da5c519787 100644 --- a/src/core/EffectChain.cpp +++ b/src/core/EffectChain.cpp @@ -25,6 +25,7 @@ #include +#include #include "EffectChain.h" #include "Effect.h" @@ -162,6 +163,7 @@ void EffectChain::moveDown( Effect * _effect ) if (_effect != m_effects.back()) { auto it = std::find(m_effects.begin(), m_effects.end(), _effect); + assert(it != m_effects.end()); std::swap(*std::next(it), *it); } } @@ -174,6 +176,7 @@ void EffectChain::moveUp( Effect * _effect ) if (_effect != m_effects.front()) { auto it = std::find(m_effects.begin(), m_effects.end(), _effect); + assert(it != m_effects.end()); std::swap(*std::prev(it), *it); } } diff --git a/src/core/Mixer.cpp b/src/core/Mixer.cpp index e14660e1fa3..59c2dd72e16 100644 --- a/src/core/Mixer.cpp +++ b/src/core/Mixer.cpp @@ -394,8 +394,8 @@ void Mixer::moveChannelLeft( int index ) else if (m_lastSoloed == b) { m_lastSoloed = a; } // go through every instrument and adjust for the channel index change - TrackContainer::TrackList songTrackList = Engine::getSong()->tracks(); - TrackContainer::TrackList patternTrackList = Engine::patternStore()->tracks(); + const TrackContainer::TrackList& songTrackList = Engine::getSong()->tracks(); + const TrackContainer::TrackList& patternTrackList = Engine::patternStore()->tracks(); for (const auto& trackList : {songTrackList, patternTrackList}) { diff --git a/src/core/PatternStore.cpp b/src/core/PatternStore.cpp index c5a3521398a..6af434f65b3 100644 --- a/src/core/PatternStore.cpp +++ b/src/core/PatternStore.cpp @@ -61,7 +61,7 @@ bool PatternStore::play(TimePos start, fpp_t frames, f_cnt_t offset, int clipNum start = start % (lengthOfPattern(clipNum) * TimePos::ticksPerBar()); - TrackList tl = tracks(); + const TrackList& tl = tracks(); for (Track * t : tl) { if (t->play(start, frames, offset, clipNum)) @@ -117,7 +117,7 @@ int PatternStore::numOfPatterns() const void PatternStore::removePattern(int pattern) { - TrackList tl = tracks(); + const TrackList& tl = tracks(); for (Track * t : tl) { delete t->getClip(pattern); @@ -134,7 +134,7 @@ void PatternStore::removePattern(int pattern) void PatternStore::swapPattern(int pattern1, int pattern2) { - TrackList tl = tracks(); + const TrackList& tl = tracks(); for (Track * t : tl) { t->swapPositionOfClips(pattern1, pattern2); @@ -159,7 +159,7 @@ void PatternStore::updatePatternTrack(Clip* clip) void PatternStore::fixIncorrectPositions() { - TrackList tl = tracks(); + const TrackList& tl = tracks(); for (Track * t : tl) { for (int i = 0; i < numOfPatterns(); ++i) @@ -215,7 +215,7 @@ void PatternStore::updateComboBox() void PatternStore::currentPatternChanged() { // now update all track-labels (the current one has to become white, the others gray) - TrackList tl = Engine::getSong()->tracks(); + const TrackList& tl = Engine::getSong()->tracks(); for (Track * t : tl) { if (t->type() == Track::Type::Pattern) @@ -230,7 +230,7 @@ void PatternStore::currentPatternChanged() void PatternStore::createClipsForPattern(int pattern) { - TrackList tl = tracks(); + const TrackList& tl = tracks(); for (Track * t : tl) { t->createClipsForPattern(pattern); diff --git a/src/core/RenderManager.cpp b/src/core/RenderManager.cpp index 969cad15bed..9f619203903 100644 --- a/src/core/RenderManager.cpp +++ b/src/core/RenderManager.cpp @@ -97,7 +97,7 @@ void RenderManager::renderNextTrack() // Render the song into individual tracks void RenderManager::renderTracks() { - const TrackContainer::TrackList & tl = Engine::getSong()->tracks(); + const TrackContainer::TrackList& tl = Engine::getSong()->tracks(); // find all currently unnmuted tracks -- we want to render these. for (const auto& tk : tl) @@ -112,7 +112,7 @@ void RenderManager::renderTracks() } } - const TrackContainer::TrackList t2 = Engine::patternStore()->tracks(); + const TrackContainer::TrackList& t2 = Engine::patternStore()->tracks(); for (const auto& tk : t2) { Track::Type type = tk->type(); diff --git a/src/core/Song.cpp b/src/core/Song.cpp index e8073f225f9..3a735331c64 100644 --- a/src/core/Song.cpp +++ b/src/core/Song.cpp @@ -383,7 +383,7 @@ void Song::processAutomations(const TrackList &tracklist, TimePos timeStart, fpp } values = container->automatedValuesAt(timeStart, clipNum); - TrackList tracks = container->tracks(); + const TrackList& tracks = container->tracks(); Track::clipVector clips; for (Track* track : tracks) diff --git a/src/gui/MixerView.cpp b/src/gui/MixerView.cpp index 0edebcb8a94..dff19ca3eb7 100644 --- a/src/gui/MixerView.cpp +++ b/src/gui/MixerView.cpp @@ -248,8 +248,8 @@ void MixerView::refreshDisplay() // update the and max. channel number for every instrument void MixerView::updateMaxChannelSelector() { - TrackContainer::TrackList songTracks = Engine::getSong()->tracks(); - TrackContainer::TrackList patternStoreTracks = Engine::patternStore()->tracks(); + const TrackContainer::TrackList& songTracks = Engine::getSong()->tracks(); + const TrackContainer::TrackList& patternStoreTracks = Engine::patternStore()->tracks(); for (const auto& trackList : {songTracks, patternStoreTracks}) { diff --git a/src/gui/clips/ClipView.cpp b/src/gui/clips/ClipView.cpp index 7a7a19c1119..de7690d26e1 100644 --- a/src/gui/clips/ClipView.cpp +++ b/src/gui/clips/ClipView.cpp @@ -25,6 +25,7 @@ #include "ClipView.h" #include +#include #include #include @@ -545,6 +546,7 @@ DataFile ClipView::createClipDataFiles( // Insert into the dom under the "clips" element Track* clipTrack = clipView->m_trackView->getTrack(); int trackIndex = std::distance(tc->tracks().begin(), std::find(tc->tracks().begin(), tc->tracks().end(), clipTrack)); + assert(trackIndex != tc->tracks().size()); QDomElement clipElement = dataFile.createElement("clip"); clipElement.setAttribute( "trackIndex", trackIndex ); clipElement.setAttribute( "trackType", static_cast(clipTrack->type()) ); @@ -1308,7 +1310,7 @@ void ClipView::mergeClips(QVector clipvs) continue; } - NoteVector currentClipNotes = mcView->getMidiClip()->notes(); + const NoteVector& currentClipNotes = mcView->getMidiClip()->notes(); TimePos mcViewPos = mcView->getMidiClip()->startPosition(); for (Note* note: currentClipNotes) diff --git a/src/gui/editors/PatternEditor.cpp b/src/gui/editors/PatternEditor.cpp index 229c90bc234..5237690a7e7 100644 --- a/src/gui/editors/PatternEditor.cpp +++ b/src/gui/editors/PatternEditor.cpp @@ -69,7 +69,7 @@ void PatternEditor::cloneSteps() void PatternEditor::removeSteps() { - TrackContainer::TrackList tl = model()->tracks(); + const TrackContainer::TrackList& tl = model()->tracks(); for (const auto& track : tl) { @@ -176,7 +176,7 @@ void PatternEditor::updatePosition() void PatternEditor::makeSteps( bool clone ) { - TrackContainer::TrackList tl = model()->tracks(); + const TrackContainer::TrackList& tl = model()->tracks(); for (const auto& track : tl) { diff --git a/src/gui/editors/PianoRoll.cpp b/src/gui/editors/PianoRoll.cpp index 05900bf0ba9..b3833599583 100644 --- a/src/gui/editors/PianoRoll.cpp +++ b/src/gui/editors/PianoRoll.cpp @@ -741,10 +741,10 @@ void PianoRoll::fitNoteLengths(bool fill) { if (!hasValidMidiClip()) { return; } m_midiClip->addJournalCheckPoint(); + m_midiClip->rearrangeAllNotes(); // Reference notes - NoteVector refNotes = m_midiClip->notes(); - std::sort(refNotes.begin(), refNotes.end(), Note::lessThan); + const NoteVector& refNotes = m_midiClip->notes(); // Notes to edit NoteVector notes = getSelectedNotes(); @@ -762,7 +762,7 @@ void PianoRoll::fitNoteLengths(bool fill) } int length; - NoteVector::iterator ref = refNotes.begin(); + auto ref = refNotes.begin(); for (Note* note : notes) { // Fast forward to next reference note @@ -797,14 +797,11 @@ void PianoRoll::constrainNoteLengths(bool constrainMax) if (!hasValidMidiClip()) { return; } m_midiClip->addJournalCheckPoint(); - NoteVector notes = getSelectedNotes(); - if (notes.empty()) - { - notes = m_midiClip->notes(); - } + const NoteVector selectedNotes = getSelectedNotes(); + const auto& notes = selectedNotes.empty() ? m_midiClip->notes() : selectedNotes; - TimePos bound = m_lenOfNewNotes; // will be length of last note - for (Note* note : notes) + TimePos bound = m_lenOfNewNotes; // will be length of last note + for (auto note : notes) { if (constrainMax ? note->length() > bound : note->length() < bound) { @@ -1207,11 +1204,11 @@ void PianoRoll::shiftSemiTone(int amount) //Shift notes by amount semitones auto selectedNotes = getSelectedNotes(); //If no notes are selected, shift all of them, otherwise shift selection - if (selectedNotes.empty()) { return shiftSemiTone(m_midiClip->notes(), amount); } - else { return shiftSemiTone(selectedNotes, amount); } + if (selectedNotes.empty()) { shiftSemiTone(m_midiClip->notes(), amount); } + else { shiftSemiTone(selectedNotes, amount); } } -void PianoRoll::shiftSemiTone(NoteVector notes, int amount) +void PianoRoll::shiftSemiTone(const NoteVector& notes, int amount) { m_midiClip->addJournalCheckPoint(); for (Note *note : notes) { note->setKey( note->key() + amount ); } @@ -1232,11 +1229,11 @@ void PianoRoll::shiftPos(int amount) //Shift notes pos by amount auto selectedNotes = getSelectedNotes(); //If no notes are selected, shift all of them, otherwise shift selection - if (selectedNotes.empty()) { return shiftPos(m_midiClip->notes(), amount); } - else { return shiftPos(selectedNotes, amount); } + if (selectedNotes.empty()) { shiftPos(m_midiClip->notes(), amount); } + else { shiftPos(selectedNotes, amount); } } -void PianoRoll::shiftPos(NoteVector notes, int amount) +void PianoRoll::shiftPos(const NoteVector& notes, int amount) { m_midiClip->addJournalCheckPoint(); diff --git a/src/gui/tracks/TrackContentWidget.cpp b/src/gui/tracks/TrackContentWidget.cpp index 442d717bfd1..619eff8317b 100644 --- a/src/gui/tracks/TrackContentWidget.cpp +++ b/src/gui/tracks/TrackContentWidget.cpp @@ -344,7 +344,7 @@ bool TrackContentWidget::canPasteSelection( TimePos clipPos, const QMimeData* md const int initialTrackIndex = tiAttr.value().toInt(); // Get the current track's index - const TrackContainer::TrackList tracks = t->trackContainer()->tracks(); + const TrackContainer::TrackList& tracks = t->trackContainer()->tracks(); const auto currentTrackIt = std::find(tracks.begin(), tracks.end(), t); const int currentTrackIndex = currentTrackIt != tracks.end() ? std::distance(tracks.begin(), currentTrackIt) : -1; @@ -443,7 +443,7 @@ bool TrackContentWidget::pasteSelection( TimePos clipPos, const QMimeData * md, TimePos grabbedClipPos = clipPosAttr.value().toInt(); // Snap the mouse position to the beginning of the dropped bar, in ticks - const TrackContainer::TrackList tracks = getTrack()->trackContainer()->tracks(); + const TrackContainer::TrackList& tracks = getTrack()->trackContainer()->tracks(); const auto currentTrackIt = std::find(tracks.begin(), tracks.end(), getTrack()); const int currentTrackIndex = currentTrackIt != tracks.end() ? std::distance(tracks.begin(), currentTrackIt) : -1; diff --git a/src/tracks/MidiClip.cpp b/src/tracks/MidiClip.cpp index b35979f61f8..490f6e6d041 100644 --- a/src/tracks/MidiClip.cpp +++ b/src/tracks/MidiClip.cpp @@ -305,7 +305,7 @@ void MidiClip::setStep( int step, bool enabled ) -void MidiClip::splitNotes(NoteVector notes, TimePos pos) +void MidiClip::splitNotes(const NoteVector& notes, TimePos pos) { if (notes.empty()) { return; } @@ -472,7 +472,7 @@ MidiClip * MidiClip::nextMidiClip() const MidiClip * MidiClip::adjacentMidiClipByOffset(int offset) const { - std::vector clips = m_instrumentTrack->getClips(); + auto& clips = m_instrumentTrack->getClips(); int clipNum = m_instrumentTrack->getClipNum(this); if (clipNum < 0 || clipNum > clips.size() - 1) { return nullptr; } return dynamic_cast(clips[clipNum + offset]);