microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.42k stars 8.3k forks source link

Guidance around const by-value function parameters #1014

Open dlong11 opened 5 years ago

dlong11 commented 5 years ago

Clarification is needed around const by-value function parameters. I noticed in a lot of the code, by-value function parameters are marked const. Some code doesn't follow this rule though.

Examples in interactivity/win32/Clipboard.cpp

void Clipboard::Copy(bool fAlsoCopyHtml)

void Clipboard::StoreSelectionToClipboard(bool const fAlsoCopyHtml)

In the CppCoreGuidelines Con.1: By default, make objects immutable

They have an exception Exception Function arguments are rarely mutated, but also rarely declared const. To avoid confusion and lots of false positives, don't enforce this rule for function arguments.

void g(const int i); // pedantic

What is the team's stance on this?

This issue is just a reminder when we create the styleguide in #890 to include something around the team's rule.

dlong11 commented 5 years ago

I just noticed another minor issue with the posted code from Clipboard.cpp. The StoreSelectionToClipboard method doesn't follow NL.26: Use conventional const notation.

adiviness commented 5 years ago

Generally we put const wherever we can, including function parameters. The exception to that is if the param is going to be modified by the function, it makes more sense to modify the copied by value variable than to make an internal copy to modify.

I’m of the opinion that we should follow the conventional const notation.

binarycrusader commented 5 years ago

I have to agree with @adiviness, I apply const liberally everywhere I can. const declares intent and the compiler enforces that intent. I know not everyone loves it, but for me it's proved helpful.

dlong11 commented 5 years ago

I have no opinion on this. I just want to make sure this kind of stuff is added to the style guide if this is the team's rule. Having this expectation documented helps when writing and reviewing code. It is so much easier to say "The style guide suggests using const on by-value function params ....." Prevents bikeshedding. You guys can discuss whether this is a rule, recommendation, preference, etc... 😄

dlong11 commented 5 years ago

I missed linking this to the Coding Style issue. #890

zadjii-msft commented 1 year ago

I'm tempted to just close this and fold it back up into "we should write some coding style docs"...