Use ptr#NAME whenever the return value is stored

[daschuer]

This is a follow up from #2601, making use of the new function

[uklotzde]

I disagree to replace all local, mutable references with pointers. That doesn't make any sense to me.

[uklotzde]

This is the only application that I had in mind and that actually makes sense to me.

[uklotzde]

Why all those changes???

[daschuer]

Regarding non const references, there are two mutually exclusive styles established:

1.) Google/Qt: "References can be confusing, as they have value syntax but pointer semantics."
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

2.) Cpp Core Guidelines: "This makes it clear to callers that the object is assumed to be modified"
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f17-for-in-out-parameters-pass-by-reference-to-non-const

You find a lot of threads where developer argue for one or the other.
Let's not do It here. There are good reasons for both.

Since we follow the Google style, I have changed the occurrences of non-const references to keep the reliability that fore instance a
a = 5;
has no side effect.

[uklotzde]

Regarding non const references, there are two mutually exclusive styles established:

1.) Google/Qt: "References can be confusing, as they have value syntax but pointer semantics."
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

2.) Cpp Core Guidelines: "This makes it clear to callers that the object is assumed to be modified"
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f17-for-in-out-parameters-pass-by-reference-to-non-const

You find a lot of threads where developer argue for one or the other.
Let's not do It here. There are good reasons for both.

Since we follow the Google style, I have changed the occurrences of non-const references to keep the reliability that fore instance a
a = 5;
has no side effect.

I never questioned the style to pass pointers instead of mutable references for out arguments. This is what the citations refer to.

But where does the Google style guide refer to mutable references that are returned as results from simple, mutable accessor functions?

[daschuer]

References can be confusing, as they have value syntax but pointer semantics.

This applies to any non-const reference variable independent if it was initialized as Funktion parameter or from a function return value.

[uklotzde]

So if I decide to store the result of a QMap::iterator member function in a local variable with a descriptive name for documentation purposes and comprehension I have to apply the & operator to obtain a pointer??

[daschuer]

You mean T &iterator::value() const;?
I think it is a good advice to always use it like i.value() = "Hallo" and not store the reference to the value at all.
I cannot think of problem where you need to store it, because i is internally already a pointer to a QMap node and very light weight. You can even user the operator -> to alter the value directly like:
i->append("Hallo");

[uklotzde]

You mean T &iterator::value() const;?
I think it is a good advice to always use it like i.value() = "Hallo" and not store the reference to the value at all.
I cannot think of problem where you need to store it, because i is internally already a pointer to a QMap node and very light weight. You can even user the operator -> to alter the value directly like:
i->append("Hallo");

Please let us keep those performance topics out of the discussion. They are completely irrelevant.

I am referring to iter.value() which doesn't reveal neither the type nor any meaning. Binding this to a local variable helps to improve readability. Do I really have to use the awkward &iter.value() and bind it to a non-nullable pointer??

[daschuer]

Yes, in that case.

But I think this discussion is not relevant, because I have just skimmed through the Mixxx code and found no occourence where the return value of T& value() and friends is stored as a T&. I don't even find a place where we use value() and friends for an assignment like this iter.value() = "Hallo"

This PR is just about improve the readability in the sense of the Google style:

- newTrackInfo.setISRC("isrc");
+ pNewTrackInfo->setISRC("isrc");

It was not my intention to discuss if this is sensible or not.

[uklotzde]

I am not convinced. But in order to move one I will accept these changes. They will cause some rework and resolving merge conflicts on my side now.

[daschuer]

Oh, I am sorry.
Thank you for approval.
Merge?

[uklotzde]

Some of the indents don't seem to be correct, but i will fix this when needed.