Beats: Make return value of mutation methods optional #4409

Holzhaus · 2021-10-13T11:54:55Z

This allows the caller to handle the failure case more efficiently, e.g. by exiting early if nothing changes.

If the caller does not care wether the operation worked and resulted in changes or not and simply needs a valid beats pointer, the following expression can be used:

pBeats->scale(...).value_or(pBeats)

Original PR description

~~Unsure if this is desired as it increases the risk for crashes (i.e. if the caller forgets to check for nullptr) and makes the code slightly less readable.~~

On the other hand it allows skipping code execution if nothing changed. This will become more relevant with #4255 (which only allows scaling if the scaled number of beats between markers is an integer).

uklotzde · 2021-10-13T15:57:18Z

@daschuer I also stumbled over this. Returning a nullptr instead of the current, unmodified instance is dangerous IMHO. But I don't know why it has been designed this way.

Every mutating operation should return a valid instance, even if it failed and leaves the instance unchanged. Otherwise the calling code becomes too complicated.

Holzhaus · 2021-10-13T16:07:32Z

@daschuer I also stumbled over this. Returning a nullptr instead of the current, unmodified instance is dangerous IMHO. But I don't know why it has been designed this way.

Every mutating operation should return a valid instance, even if it failed and leaves the instance unchanged. Otherwise the calling code becomes too complicated.

Hmm, the alternative to returning a nullptr would be to add an optional bool* ok parameter that is set to false in the failure case.

daschuer · 2021-10-13T18:34:26Z

I prefere also to always return a valid pointer.
The bool* ok sounds reasonable. If we default it to nullptr we can pass it only if we have a something to do in the failure case.

Holzhaus · 2021-10-13T22:08:33Z

I just implemented that and tbh it makes the code even more complicated. We only use the scale method in two locations (WTrackMenu and the track dialog) and in both we do want to know if scaling worked. So I think just returning nullptr would be simpler. But I don't really care, we can also close this.

daschuer · 2021-10-13T22:53:43Z

src/library/dlgtrackinfo.cpp

+    const auto pBeatsScaled = m_pBeatsClone->scale(bpmScale, &ok);
+    if (!ok) {
+        return;
+    }
+    m_pBeatsClone = pBeatsScaled;
    updateSpinBpmFromBeats();


I had something like this in mind:

Suggested change

const auto pBeatsScaled = m_pBeatsClone->scale(bpmScale, &ok);

if (!ok) {

return;

}

m_pBeatsClone = pBeatsScaled;

updateSpinBpmFromBeats();

m_pBeatsClone = m_pBeatsClone->scale(bpmScale, &ok);

if (ok) {

updateSpinBpmFromBeats();

}

daschuer · 2021-10-13T22:57:01Z

I just implemented that and tbh it makes the code even more complicated.

I don't have a strong opinion here. The current solution looks good.
@uklotzde What do you think?

uklotzde · 2021-10-13T23:54:50Z

Returning a nullptr as an indicator for an error is dangerous, not intuitive, and conceptually wrong. Unfortunately, we are hitting some C++ limitations here with this design.

As the caller I want to reassign the pointer with every call independent of the outcome. On errors the last state (= pointer) should be preserved and information about the error should be returned separately. Otherwise I need to juggle pointers on every fallible operation.

uklotzde · 2021-10-14T06:34:16Z

The only alternative for out parameters would be exceptions. But exceptions strike loopholes into the type system. Don't use them for anything but fatal errors.

Instead of out parameters you could return a pair. Not sure if structured binding in C++17 is sophisticated enough to allow writing readable code.

daschuer · 2021-10-14T06:38:56Z

I consider using std::pair as an anti pattern in terms of readability.

uklotzde · 2021-10-14T07:30:53Z

I consider using std::pair as an anti pattern in terms of readability.

This is your personal opinion. Tell that the Go folks or consider any other modern programming language that supports anonymous tuples out-of-the box, including C++17. It works considerably well in strongly typed languages, even if I don't like how error handling is done in Go. We should not rule out this option unconditionally for the whole project.

Holzhaus · 2021-10-14T07:37:40Z

I think the semantically correct way would be using std::optional.

If you dont care if scaling worked or not, just use pBeats->scaled(...).value_or(pBeats).

uklotzde · 2021-10-14T09:08:22Z

I think the semantically correct way would be using std::optional.

If you dont care if scaling worked or not, just use pBeats->scaled(...).value_or(pBeats).

I didn't consider this option, seems to be convenient to use for preserving the existing state in case of a failure. Go for it.

daschuer · 2021-10-14T19:36:32Z

In my opinion this is now the most complex, solution and not the best fit. std:::optional is useful if the returned value has no null value. A pointer already has a null value.

The nice thing of the original and the pOK solution was that you can use unconditional
m_pBeatsClone = m_pBeatsClone->scale(bpmScale)
This simplicity and safety is now lost.

uklotzde · 2021-10-14T19:50:55Z

src/track/beatgrid.cpp

@@ -261,9 +261,9 @@ mixxx::Bpm BeatGrid::getBpmAroundPosition(audio::FramePos position, int n) const
    return getBpm();
 }

-BeatsPointer BeatGrid::translate(audio::FrameDiff_t offset) const {
+std::optional<BeatsPointer> BeatGrid::translate(audio::FrameDiff_t offset) const {


Renaming these methods to tryTranslate() and adding an infallible translate() method might resolve @daschuer's concerns. The other option would be to add a translateInfallible() single-line function, but I like the trySomething() naming more for emphasizing fallibility.

std::option is required to prevent unconsciously using the return value which might be a nullptr. This is how you catch bugs at compile time instead at runtime!

I considered that, too. But it's kind of pointless to add methods that are never used IMHO.

I can do it @daschuer insists, even though it's dead code.

Please don't add methods until/unless they are needed.

Renaming the fallible methods to trySomething() might still help to easily distinguish them at the call site without needing to look up their definition.

If the infallible variants are not needed/used then don't add them.

daschuer · 2021-10-14T20:24:20Z

I can do it @daschuer insists, even though it's dead code.

I don't insist anything in this discussion. I have been jumped in because I was mentioned here. All these solutions are working for me. I have only expressed my personal opinion, else I would have issued a formal review.

But I actually like the proposed name, if we not always return a valid pointer. Keeping unused functions with the old name does not make sense.

Holzhaus · 2021-10-15T08:39:17Z

I don't insist anything in this discussion. I have been jumped in because I was mentioned here. All these solutions are working for me. I have only expressed my personal opinion, else I would have issued a formal review.

Yeah sorry, i wasn't meaning to say you were stubborn or something, I just wanted to say that if you'd prefer to add the unused infallible methods I could do that.

Just trying to find a solution here that everyone can live with :D

In my opinion this is now the most complex, solution and not the best fit. std:::optional is useful if the returned value has no null value. A pointer already has a null value.

I agree that using std::optional to wrap a pointer seems weird at first, but I think this approach has several advantages the alternatives (using nullptr, using an pointer argument for output).

First and foremost, it's semantically correct (the return value is literally optional).

In contrast to the nullptr solution, it's not possible to accidently segfault mixxx by forgetting to check for a nullptr return value, because the type is different. It will cause a compiler warning.

In contrast to the pointer argument solution, both the method implementation and the calling code are much shorter and nicer to read IMHO.

The nice thing of the original and the pOK solution was that you can use unconditional
 m_pBeatsClone = m_pBeatsClone->scale(bpmScale)
This simplicity and safety is now lost.

If you're not interested in the failure case, the code only becomes slightly longer:

 m_pBeatsClone = m_pBeatsClone->tryScale(bpmScale).value_or(m_pBeatsClone)

daschuer · 2021-10-15T08:45:43Z

OK, if you now rename the function with the try prefix we have a good solution.

This allows the caller to handle the failure case more efficiently, e.g. by exiting early if nothing changes. If the caller does not care wether the operation worked and resulted in changes or not and simply needs a valid beats pointer, the following expression can be used: pBeats->tryScale(...).value_or(pBeats)

daschuer · 2021-10-15T08:55:37Z

By the way: We can get rid of all these boilerplate code around pointers if we turn the Beatpointer into a copy on write Beats class that takes the a const pointer internally like QByteArray.

Holzhaus · 2021-10-15T09:06:12Z

src/library/dlgtrackinfo.cpp

@@ -610,7 +612,7 @@ void DlgTrackInfo::slotSpinBpmValueChanged(double value) {
        if (oldValue == bpm) {
            return;
        }
-        m_pBeatsClone = m_pBeatsClone->setBpm(bpm);
+        m_pBeatsClone = m_pBeatsClone->trySetBpm(bpm).value_or(m_pBeatsClone);


Here is the only occurrence where the optional is not used. Unsure if we want to add an infallible setBpm just for that, especially because it would be inconsistent with the other beats mutation methods. If we add it, maybe we should add it for the other ones for consistency. Or we leave it like this. I don't really care, let me know what you prefer.

I don't mind as well.

daschuer

LGTM, thank you!

daschuer · 2021-10-15T09:51:55Z

src/library/dlgtrackinfo.cpp

@@ -610,7 +612,7 @@ void DlgTrackInfo::slotSpinBpmValueChanged(double value) {
        if (oldValue == bpm) {
            return;
        }
-        m_pBeatsClone = m_pBeatsClone->setBpm(bpm);
+        m_pBeatsClone = m_pBeatsClone->trySetBpm(bpm).value_or(m_pBeatsClone);


I don't mind as well.

Holzhaus added this to the 2.4.0 milestone Oct 13, 2021

github-actions bot added library ui labels Oct 13, 2021

Holzhaus force-pushed the beats-scale-nullptr branch from c0aeff1 to 08ff362 Compare October 13, 2021 13:57

Holzhaus changed the title ~~Beats: Avoid unnecessary copies if scale() fails~~ Beats: Return nullptr if scale() fails Oct 13, 2021

daschuer reviewed Oct 13, 2021

View reviewed changes

Holzhaus force-pushed the beats-scale-nullptr branch from b70919a to bfb9cc7 Compare October 14, 2021 10:16

Holzhaus changed the title ~~Beats: Return nullptr if scale() fails~~ Beats: Make return value of mutation methods optional Oct 14, 2021

github-actions bot added the code quality label Oct 14, 2021

uklotzde reviewed Oct 14, 2021

View reviewed changes

Holzhaus force-pushed the beats-scale-nullptr branch from bfb9cc7 to 8534357 Compare October 15, 2021 08:54

Holzhaus commented Oct 15, 2021

View reviewed changes

daschuer approved these changes Oct 15, 2021

View reviewed changes

daschuer merged commit 461b1ac into mixxxdj:main Oct 15, 2021

ronso0 mentioned this pull request Dec 28, 2021

DlgTrackInfo: ensure Beats pointer before scaling beats #4587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beats: Make return value of mutation methods optional #4409

Beats: Make return value of mutation methods optional #4409

Holzhaus commented Oct 13, 2021 •

edited

Loading

uklotzde commented Oct 13, 2021

Holzhaus commented Oct 13, 2021 •

edited

Loading

daschuer commented Oct 13, 2021

Holzhaus commented Oct 13, 2021

daschuer Oct 13, 2021

daschuer commented Oct 13, 2021

uklotzde commented Oct 13, 2021 •

edited

Loading

uklotzde commented Oct 14, 2021

daschuer commented Oct 14, 2021 •

edited

Loading

uklotzde commented Oct 14, 2021

Holzhaus commented Oct 14, 2021 •

edited

Loading

uklotzde commented Oct 14, 2021

daschuer commented Oct 14, 2021

uklotzde Oct 14, 2021

uklotzde Oct 14, 2021

Holzhaus Oct 14, 2021

Be-ing Oct 14, 2021

uklotzde Oct 14, 2021

uklotzde Oct 14, 2021

daschuer commented Oct 14, 2021

Holzhaus commented Oct 15, 2021 •

edited

Loading

daschuer commented Oct 15, 2021

daschuer commented Oct 15, 2021 •

edited

Loading

Holzhaus Oct 15, 2021

daschuer Oct 15, 2021

daschuer left a comment

daschuer Oct 15, 2021

Beats: Make return value of mutation methods optional #4409

Beats: Make return value of mutation methods optional #4409

Conversation

Holzhaus commented Oct 13, 2021 • edited Loading

Original PR description

uklotzde commented Oct 13, 2021

Holzhaus commented Oct 13, 2021 • edited Loading

daschuer commented Oct 13, 2021

Holzhaus commented Oct 13, 2021

Choose a reason for hiding this comment

daschuer commented Oct 13, 2021

uklotzde commented Oct 13, 2021 • edited Loading

uklotzde commented Oct 14, 2021

daschuer commented Oct 14, 2021 • edited Loading

uklotzde commented Oct 14, 2021

Holzhaus commented Oct 14, 2021 • edited Loading

uklotzde commented Oct 14, 2021

daschuer commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Oct 14, 2021

Holzhaus commented Oct 15, 2021 • edited Loading

daschuer commented Oct 15, 2021

daschuer commented Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Oct 13, 2021 •

edited

Loading

Holzhaus commented Oct 13, 2021 •

edited

Loading

uklotzde commented Oct 13, 2021 •

edited

Loading

daschuer commented Oct 14, 2021 •

edited

Loading

Holzhaus commented Oct 14, 2021 •

edited

Loading

Holzhaus commented Oct 15, 2021 •

edited

Loading

daschuer commented Oct 15, 2021 •

edited

Loading