Re: C++ "good practices" regarding constifying a function parameter?

[Scott Kostyshak]

On Sun, Aug 07, 2016 at 12:48:35PM +0200, Abdelrazak Younes wrote:

> I think one should think about c++11 as a new language if you really want to
> take advantage of it. auto, range loop, new algorithm, etc are very nice but
> mixing old style c++ and c++11 leads to confusing source code.

Regarding this comment and the conversation in general, I'm happy to
follow any style guidelines, but it would be nice to have it written
down. I think currently we just tell newcommers "try to make your code
look like the code around it", which is a good general rule but I think
we could do better. Can we just copy another style guide and tweak it as
we go along?

Scott


signature.asc
Description: PGP signature

Re: C++ "good practices" regarding constifying a function parameter?

[Abdelrazak Younes]

On 06/08/2016 21:58, Guillaume Munch wrote:

But I agree that new move functionality in C++11 is really nice and I

know that I have to unlearn the good practice I learned so many years
ago in order to use them efficiently.


The issue with c++11 move semantics is that it is an addition of a big
layer of complexity on top of the rest for backward-compatibility
reasons and that makes everything very complicated. We want to avoid
needlessly writing complicated code that few people understand.


Agreed. Indeed I've seen code with lots of lambda that is very hard to read.

I think one should think about c++11 as a new language if you really 
want to take advantage of it. auto, range loop, new algorithm, etc are 
very nice but mixing old style c++ and c++11 leads to confusing source code.



The most important thing to remember is the conclusion in part 3: pass
by const reference, unless the function needs a copy "conceptually". In
this case, you should pass by value. Pros:
* Simple and generic rule.
* Not a big change compared to what developers are used to.
* Exposes in the interface the knowledge of when passing an argument is
going to be expensive.
* In the latter case, offers to the caller the opportunity to move the
argument instead of copying it.


Sounds good.




***


In addition, now, what if we want to design a bulky class that relies on
being moved more often than being copied? It is possible to simply make
the copy explicit in the class declaration (say TexRow), as follows:

explicit TexRow(TexRow const & other) = default;
TexRow(TexRow && other) = default;
TexRow & operator=(TexRow const & other) = default;
TexRow & operator=(TexRow && other) = default;

(this defines respectively the copy and move constructors, and the 
copy and move operators, as default.)

Making the copy constructor explicit prevents implicit copies when
passing an argument. One has to explicitly move or copy. Pros:

* It forces to think about it.
* It also prevents every cases of move(obj) unexpectedly performing a
copy instead of a move.

I still have to see whether this particular use case is not redundant
with encapsulating in a unique_ptr. But I would like to suggest that
copy constructors should be made explicit when a class is designed to be
moved, if only to avoid reading code containing move(obj) and not
knowing whether it's really a move.


Sounds good as well :-)

Abdel.

Re: C++ "good practices" regarding constifying a function parameter?

[Guillaume Munch]

Le 06/08/2016 à 16:45, Abdelrazak Younes a écrit :

On 06/08/2016 17:04, Guillaume Munch wrote:

Le 06/08/2016 à 10:16, Abdelrazak Younes a écrit :


(1) Do not commit any part of the patch because it is so minor.


This is my preference because it doesn't bring anything and it can
create confusion for the beginner when compared with const
reference.


But I am sceptical of a code guideline that discourages documenting
the code (for instance with const) when one feels that it is appropriate.


I'd rather this guy take the time to write down a nice doxygen comment
in the header to explain the parameter.


The compliance with the const annotation is checked by the compiler. I
do not think that any amount of English comments can match that.

That being said, I should learn how to doxygen, because up to now I have
mostly been imitating the style of comments that I saw.




The coding style should also say that function parameter shall not
be changed in the function. Create your own local copy if you need
it.



I know where this comes from, but this surely cannot be a rule anymore
since this defeats any benefit from move operators.


AFAIU move operator has zero benefits on POD, copy will happen; it only
works on classes or containers that support it (eg vector).


I agree with you on this: for PODs, move is the same as copy; and also
when they are small we want to pass them by value independently from
this consideration as you said.


But to be
honest I prefer that complex types are always using const ref or rvalue
(|&&) |if you want to have move semantics.


I have seen that this topic was already much debated among C++11
people. The problem with passing by rvalue reference (&&) is that
you end up having to duplicate your function definitions. Actually not,
you have to write (up to) 2ⁿ definitions where n is the number of
arguments which you want to benefit from moves. I would not recommend
this approach except for very specific cases where this granularity is
necessary.




I prefer the style
where a function that only works on a copy receives the argument by
value.


I prefer to choose explicitly whether to use copy or const ref :-)


Not only this is simpler to understand as a principle and in an
interface, but it leaves the compiler (and when necessary, the
programmer) the possibility of moving the argument instead of copying it.


And it can kill the performance if you pass a big variable that is not
movable,  AFAIU (I may be wrong).


Of course, as I said, that would only be in the case where the function
needs its own copy anyway. (Typically, a constructor parameter that gets
copied into a member, and that now can be moved instead.)



But I agree that new move functionality in C++11 is really nice and I
know that I have to unlearn the good practice I learned so many years
ago in order to use them efficiently.


The issue with c++11 move semantics is that it is an addition of a big
layer of complexity on top of the rest for backward-compatibility
reasons and that makes everything very complicated. We want to avoid
needlessly writing complicated code that few people understand.

There are also pitfalls, for instance you can too easily write move(obj)
and the compiler performs a copy (!) instead. (Just make obj const. No
compiler error!) So it is not very (or very not) beginner-friendly. (And
I won't get into the details of universal references and perfect
forwarding where && suddenly changes meaning.) But below are some rules
that we can follow to make things easier.

When I was documenting myself on this issue I found that the following
series of blog posts nicely summarizes it, esp. the 1st and the 3rd
posts (for people who want to get into the details with examples):

http://www.codesynthesis.com/~boris/blog/2012/06/19/efficient-argument-passing-cxx11-part1/
http://www.codesynthesis.com/~boris/blog/2012/06/26/efficient-argument-passing-cxx11-part2/
http://www.codesynthesis.com/~boris/blog/2012/07/03/efficient-argument-passing-cxx11-part3/


The most important thing to remember is the conclusion in part 3: pass
by const reference, unless the function needs a copy "conceptually". In
this case, you should pass by value. Pros:
* Simple and generic rule.
* Not a big change compared to what developers are used to.
* Exposes in the interface the knowledge of when passing an argument is
going to be expensive.
* In the latter case, offers to the caller the opportunity to move the
argument instead of copying it.


***


In addition, now, what if we want to design a bulky class that relies on
being moved more often than being copied? It is possible to simply make
the copy explicit in the class declaration (say TexRow), as follows:

explicit TexRow(TexRow const & other) = default;
TexRow(TexRow && other) = default;
TexRow & operator=(TexRow const & other) = default;
TexRow & operator=(TexRow && other) = default;

(this defines respectively the copy and move constructors, and 

Re: C++ "good practices" regarding constifying a function parameter?

[Abdelrazak Younes]

On 06/08/2016 17:04, Guillaume Munch wrote:

Le 06/08/2016 à 10:16, Abdelrazak Younes a écrit :


(1) Do not commit any part of the patch because it is so minor.


This is my preference because it doesn't bring anything and it can
create confusion for the beginner when compared with const
reference.


But I am sceptical of a code guideline that discourages documenting
the code (for instance with const) when one feels that it is appropriate.


I'd rather this guy take the time to write down a nice doxygen comment 
in the header to explain the parameter.





The C++ beginner could be tempted to add a "&" here...


If a beginner changes const into const & without understanding what they
are doing, then this hints at an issue which is different from
inexperience. I have to reject this argument.


Shit happens :-) And this kind of mistake are easily missed even by the 
careful reviewer.





The coding style should also say that function parameter shall not
be changed in the function. Create your own local copy if you need
it.



I know where this comes from, but this surely cannot be a rule anymore
since this defeats any benefit from move operators.


AFAIU move operator has zero benefits on POD, copy will happen; it only 
works on classes or containers that support it (eg vector). But to be 
honest I prefer that complex types are always using const ref or rvalue 
(|&&) |if you want to have move semantics.



I prefer the style
where a function that only works on a copy receives the argument by
value.


I prefer to choose explicitly whether to use copy or const ref :-)


Not only this is simpler to understand as a principle and in an
interface, but it leaves the compiler (and when necessary, the
programmer) the possibility of moving the argument instead of copying it.


And it can kill the performance if you pass a big variable that is not 
movable,  AFAIU (I may be wrong).


But I agree that new move functionality in C++11 is really nice and I 
know that I have to unlearn the good practice I learned so many years 
ago in order to use them efficiently.


Thanks,
Abdel.

Re: C++ "good practices" regarding constifying a function parameter?

[Guillaume Munch]

(For some reason the message did not get through, retrying. That's the
second time this happens in the recent days. Anyone else experiences
issues with gmane?)


Le 06/08/2016 à 10:16, Abdelrazak Younes a écrit :


(1) Do not commit any part of the patch because it is so minor.


This is my preference because it doesn't bring anything and it can
create confusion for the beginner when compared with const
reference.


But I am sceptical of a code guideline that discourages documenting
the code (for instance with const) when one feels that it is appropriate.


The C++ beginner could be tempted to add a "&" here...


If a beginner changes const into const & without understanding what they
are doing, then this hints at an issue which is different from
inexperience. I have to reject this argument.


The coding style should also say that function parameter shall not
be changed in the function. Create your own local copy if you need
it.



I know where this comes from, but this surely cannot be a rule anymore
since this defeats any benefit from move operators. I prefer the style
where a function that only works on a copy receives the argument by
value. Not only this is simpler to understand as a principle and in an
interface, but it leaves the compiler (and when necessary, the
programmer) the possibility of moving the argument instead of copying it.

Guillaume

Re: C++ "good practices" regarding constifying a function parameter?

[José Abílio Matos]

On Saturday, August 6, 2016 4:17:01 PM WEST Abdelrazak Younes wrote:
> Exactly!
> 
> Cheers,
> Abdel.

Can I use a 4K screen as the standard? ;-)

FWIW I agree with the rational of having short/not too long functions, I am 
just saying that half a screen is an ambiguous measure. :-)

-- 
José Abílio

Re: C++ "good practices" regarding constifying a function parameter?

[Abdelrazak Younes]

On 06/08/2016 14:41, Vincent van Ravesteijn wrote:
But, I guess the coding standard says that functions shouldn't be 
longer than something like half a screen. 


Exactly!

Cheers,
Abdel.

Re: C++ "good practices" regarding constifying a function parameter?

[Vincent van Ravesteijn]

Op 6 aug. 2016 11:26 schreef "Abdelrazak Younes" :
>
> Hi Scott,
>
> Too late already but my 2 cents below :-)
>
>
> On 27/07/2016 02:54, Scott Kostyshak wrote:
>>
>> The attached patch constifies a function parameter. My question is
>> whether this patch causes more pain to other developers than it does
>> good to the code.
>>
>> The patch modifies a header that is included in many of our .cpp files,
>> so will cause a significant amount of recompilation.
>>
>> I think I will come across this scenario in the future so I am curious:
>> what are your preferences?
>>
>> (1) Do not commit any part of the patch because it is so minor.
>
>
> This is my preference because it doesn't bring anything and it can create
confusion for the beginner when compared with const reference. The C++
beginner could be tempted to add a "&" here...
>
> I think it was in the coding style document somewhere that POD parameter
(except big structure of course) should be passed by copy.
>
> Conclusion: I'd prefer the coding style to actually forbids this actually
:-)
> The coding style should also say that function parameter shall not be
changed in the function. Create your own local copy if you need it.
>
> Thanks,
> Abdel.
>

Hi Abdel,

I remember the same coding rule and was therefore tempted to reply in the
same line. However, when I looked into the code, I saw that this function
was very very long, and therefore it is not easy to always see that this
variable is actually a function parameter.

But, I guess the coding standard says that functions shouldn't be longer
than something like half a screen.

Vincent

Re: C++ "good practices" regarding constifying a function parameter?

[Abdelrazak Younes]

Hi Scott,

Too late already but my 2 cents below :-)

On 27/07/2016 02:54, Scott Kostyshak wrote:

The attached patch constifies a function parameter. My question is
whether this patch causes more pain to other developers than it does
good to the code.

The patch modifies a header that is included in many of our .cpp files,
so will cause a significant amount of recompilation.

I think I will come across this scenario in the future so I am curious:
what are your preferences?

(1) Do not commit any part of the patch because it is so minor.


This is my preference because it doesn't bring anything and it can 
create confusion for the beginner when compared with const reference. 
The C++ beginner could be tempted to add a "&" here...


I think it was in the coding style document somewhere that POD parameter 
(except big structure of course) should be passed by copy.


Conclusion: I'd prefer the coding style to actually forbids this 
actually :-)
The coding style should also say that function parameter shall not be 
changed in the function. Create your own local copy if you need it.


Thanks,
Abdel.

Re: C++ "good practices" regarding constifying a function parameter?

[Scott Kostyshak]

On Fri, Jul 29, 2016 at 03:43:45PM +0100, Guillaume Munch wrote:

> All these are good reasons.

OK it's in at 60e89213.

Scott


signature.asc
Description: PGP signature

Re: C++ "good practices" regarding constifying a function parameter?

[Guillaume Munch]

Le 27/07/2016 à 22:19, Scott Kostyshak a écrit :

For the function definition, the difference is in terms of
documentation. If you are adding const in the definition because you
find it clearer this way, then it is a good reason to change it.


Yes this was my intention. To me it has the same purpose as declaring a
local variable const in the body of a function. For example, if I see
that "param" is used at the end of a function I know that "param" is a
const parameter, I don't have to read the whole function looking to see
if param was modified in the middle. In addition to readability, I think
it can in some (unlikely) situations prevent future coding mistakes.
e.g. I might write the code and at the end of the body I use "param"
making the assumption that it has the same value that it started with.
If someone (e.g. future me) comes along and changes the value in the
middle of the function they'll get a compiler error and think twice
before removing const. I think the benefits are especially useful for
long functions.

Do you also const parameters and const local variables useful for
understanding and future-proofing functions or not really?


All these are good reasons.

Re: C++ "good practices" regarding constifying a function parameter?

[Richard Heck]

On 07/27/2016 05:19 PM, Scott Kostyshak wrote:
> So to double-check, no one is against the .cpp change I proposed, right?
> (I will not commit the .h change after this helpful conversation)

Yes, I think that's right.

> Thanks for this detailed explanation, Guillaume. I think that small
> things like this help me build up a better understanding of C++.

+1

rh

Re: C++ "good practices" regarding constifying a function parameter?

[Scott Kostyshak]

On Wed, Jul 27, 2016 at 12:06:51PM +0100, Guillaume Munch wrote:

> Hi Scott,
> 
> 
> - f(…, bool b)
> + f(…, bool const b)
> 
> This being passed by value, this changes nothing for the declaration.
> The two signatures are even considered equal for overloading (if it
> came to this). So I recommend leaving the header unchanged.

Good to know. I often switch back and forth between the .cpp and .h
files to understand a group of functions. Sometimes I would read the
signature of the declaration in the .h file and then go directly to the
function body without looking at the signature of it. But now I realize
this is a bad idea because the two signatures are not necessarily the
same. const can be different and even the parameter names are not
required to be the same, so now I know that when I go to the
implementation, I should not skip over the implementation's signature.

> For the function definition, the difference is in terms of
> documentation. If you are adding const in the definition because you
> find it clearer this way, then it is a good reason to change it.

Yes this was my intention. To me it has the same purpose as declaring a
local variable const in the body of a function. For example, if I see
that "param" is used at the end of a function I know that "param" is a
const parameter, I don't have to read the whole function looking to see
if param was modified in the middle. In addition to readability, I think
it can in some (unlikely) situations prevent future coding mistakes.
e.g. I might write the code and at the end of the body I use "param"
making the assumption that it has the same value that it started with.
If someone (e.g. future me) comes along and changes the value in the
middle of the function they'll get a compiler error and think twice
before removing const. I think the benefits are especially useful for
long functions.

Do you also const parameters and const local variables useful for
understanding and future-proofing functions or not really? If no, do you
find them annoying or just don't care?

> In this case, you can see "const" as a detail of implementation that the
> user of the header does not care about. If you see it that way, then the
> difference between the declaration and the definition looks strange no
> longer.

I see, makes sense now.

> The answer would be different if the const was somewhere inside, e.g.:
> - f(…, bool & b)
> + f(…, bool const & b)
> in which case the conveyed meaning is entirely different, and it would
> be a matter of priority to correctly document the declaration.

Indeed, I do see the difference.

So to double-check, no one is against the .cpp change I proposed, right?
(I will not commit the .h change after this helpful conversation)

Thanks for this detailed explanation, Guillaume. I think that small
things like this help me build up a better understanding of C++.

Scott


signature.asc
Description: PGP signature

Re: C++ "good practices" regarding constifying a function parameter?

[Guillaume Munch]

Le 27/07/2016 à 01:54, Scott Kostyshak a écrit :

The attached patch constifies a function parameter. My question is
whether this patch causes more pain to other developers than it does
good to the code.

The patch modifies a header that is included in many of our .cpp files,
so will cause a significant amount of recompilation.

I think I will come across this scenario in the future so I am curious:
what are your preferences?

(1) Do not commit any part of the patch because it is so minor.
(2) Commit the .cpp part, but not the .h part (this symmetry is not
checked by C++ since the constness only matters within the body of the
function).
(3) Commit the full patch, as is.
(4) Commit the full patch, as is. But save up these commits and push
only when I have a few of them or right after a commit that will cause
recompilation anyway.



Hi Scott,


- f(…, bool b)
+ f(…, bool const b)

This being passed by value, this changes nothing for the declaration.
The two signatures are even considered equal for overloading (if it
came to this). So I recommend leaving the header unchanged.

For the function definition, the difference is in terms of
documentation. If you are adding const in the definition because you
find it clearer this way, then it is a good reason to change it.

In this case, you can see "const" as a detail of implementation that the
user of the header does not care about. If you see it that way, then the
difference between the declaration and the definition looks strange no
longer.

The answer would be different if the const was somewhere inside, e.g.:
- f(…, bool & b)
+ f(…, bool const & b)
in which case the conveyed meaning is entirely different, and it would
be a matter of priority to correctly document the declaration.


Guillaume

Re: C++ "good practices" regarding constifying a function parameter?

[Scott Kostyshak]

On Tue, Jul 26, 2016 at 11:40:17PM -0400, Richard Heck wrote:
> On 07/26/2016 08:54 PM, Scott Kostyshak wrote:
> > The attached patch constifies a function parameter. My question is
> > whether this patch causes more pain to other developers than it does
> > good to the code.
> 
> So the change is:
> 
> -bool BufferView::scrollToCursor(DocIterator const & dit, bool recenter)
> +bool BufferView::scrollToCursor(DocIterator const & dit, bool const recenter)

That is the change in the .cpp file.

> What is the significance of this change? Since recenter is passed by
> value, why would the caller care if it is const?

I don't think the caller would care.

> Is this just a signal
> to the compiler?

Yes it is a signal to the compiler that the recenter variable should not
be written to. It is also a signal to the reader of the code.

Regarding the change in the header file, I don't think the compiler
cares about that change. I just find it strange to have the declaration
appear different from the definition.

Scott


signature.asc
Description: PGP signature

Re: C++ "good practices" regarding constifying a function parameter?

[Richard Heck]

On 07/26/2016 08:54 PM, Scott Kostyshak wrote:
> The attached patch constifies a function parameter. My question is
> whether this patch causes more pain to other developers than it does
> good to the code.

So the change is:

-bool BufferView::scrollToCursor(DocIterator const & dit, bool recenter)
+bool BufferView::scrollToCursor(DocIterator const & dit, bool const recenter)

What is the significance of this change? Since recenter is passed by
value, why would the caller care if it is const? Is this just a signal
to the compiler?

Richard