Mailing List Archive

On Thu, 2021-10-21 at 00:45 -0400, Scott T wrote:
> Is it necessary to pass QString by const reference?  Specifically I'm
> looking at libmythmetadata/videometadata (converting to non-PIMPL) and
> libmyth/programinfo (some unchanged by
> https://github.com/MythTV/mythtv/commit/1a8097e3249a8ed2ea051056da08a35993bdc953
>  
> )
>
> If I understand correctly, passing by value increments the reference
> counter before the function call, and then using std::move just moves
> the (pointer to the) data.  Thus, passing by value is recommended for
> constructors.

I think its six of one or half dozen of the other, with one caveat. If
you pass by const reference, nothing has to be done in the caller and
its the callee's responsibility to copy the object (which as you
pointed out is a small object and a reference count to the data). If
you pass by value, the caller has to copy the object and increment the
reference count and the callee doesn't have to do anything. The caveat
is that when the constructor implementation is in the header file and
it uses call by value and std::move, then the optimizer has more
opportunities to find things that it can optimize.

Googling will get you arguments for both approaches. The clang-tidy
docs say that now that std::move exists and many library functions have
been updated with move constructors that pass by value with std::move
is the preferred approach.

I have no objection to converting constructors to use pass-by-value and
std::move. Seems like a change that should be a standalone commit.

Clazy also has a couple of checks related to pass by value/reference:

https://github.com/KDE/clazy/blob/master/docs/checks/README-function-args-by-ref.md
https://github.com/KDE/clazy/blob/master/docs/checks/README-function-args-by-value.md

> Should QStrings be returned from a function by value or const reference
> as many are in videometadata?

I think I prefer const reference. The callee is just loading a pointer
into a register, and then its up to the caller whether or not to make a
copy of it. It you return a value, you always make a copy of the
string.

David


_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

Personally, I like to know what is happening and how, and for readers
to understand what is going on at a glance.

For QString passing by value might be okay because ultimately it's a
refcounted object with copy on write and copy is cheap.

But for others not quite so.

So passing by const reference when you don't intend a transfer of
ownership or passing by rvalue if you do , gives a much greater
clarity and makes code review easier.
And when writing copy constructors, have both, either explicitly or
with = default.

e.g.
class MyClass {
MyClass(const MyClass& other);
MyClass(MyClass&& other);
};

On Sat, 23 Oct 2021 at 02:06, David Hampton via mythtv-dev
<mythtv-dev@mythtv.org> wrote:
>
> On Thu, 2021-10-21 at 00:45 -0400, Scott T wrote:
> > Is it necessary to pass QString by const reference? Specifically I'm
> > looking at libmythmetadata/videometadata (converting to non-PIMPL) and
> > libmyth/programinfo (some unchanged by
> > https://github.com/MythTV/mythtv/commit/1a8097e3249a8ed2ea051056da08a35993bdc953
> >
> > )
> >
> > If I understand correctly, passing by value increments the reference
> > counter before the function call, and then using std::move just moves
> > the (pointer to the) data. Thus, passing by value is recommended for
> > constructors.
>
> I think its six of one or half dozen of the other, with one caveat. If
> you pass by const reference, nothing has to be done in the caller and
> its the callee's responsibility to copy the object (which as you
> pointed out is a small object and a reference count to the data). If
> you pass by value, the caller has to copy the object and increment the
> reference count and the callee doesn't have to do anything. The caveat
> is that when the constructor implementation is in the header file and
> it uses call by value and std::move, then the optimizer has more
> opportunities to find things that it can optimize.
>
> Googling will get you arguments for both approaches. The clang-tidy
> docs say that now that std::move exists and many library functions have
> been updated with move constructors that pass by value with std::move
> is the preferred approach.
>
> I have no objection to converting constructors to use pass-by-value and
> std::move. Seems like a change that should be a standalone commit.
>
> Clazy also has a couple of checks related to pass by value/reference:
>
> https://github.com/KDE/clazy/blob/master/docs/checks/README-function-args-by-ref.md
> https://github.com/KDE/clazy/blob/master/docs/checks/README-function-args-by-value.md
>
> > Should QStrings be returned from a function by value or const reference
> > as many are in videometadata?
>
> I think I prefer const reference. The callee is just loading a pointer
> into a register, and then its up to the caller whether or not to make a
> copy of it. It you return a value, you always make a copy of the
> string.
>
> David
>
>
> _______________________________________________
> mythtv-dev mailing list
> mythtv-dev@mythtv.org
> http://lists.mythtv.org/mailman/listinfo/mythtv-dev
> http://wiki.mythtv.org/Mailing_List_etiquette
> MythTV Forums: https://forum.mythtv.org
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org