Changing C++ Coding Conventions

[phosit]

Ref: https://trac.wildfiregames.com/wiki/Coding_Conventions#C

auto

Quote

In c++11, it is possible to use the auto type-specifier, wherein the appropriate data-type is determined at compile time. Although potentially useful, overuse can cause serious problems in the long-run. Therefore, this code feature should be used in moderation. Specifically:

• It should be clear from reading the code what type the auto is replacing. (Add a comment if necessary.)
• It should only ever be used as a replacement for an iterator type.
• It should only be used if the data-type-specifier it is standing in for is long. As a rule of thumb, if the line would be shorter than your chosen suitable line width (see convention on avoiding wide lines under formatting above) without it, don't use it.

This paragraph should be removed/shorted, because

• sometimes it does make the code more verbose by adding very little value,
• sometimes it is slower because of implicit conversion,
• and sometimes it is impossible lambda; structured binding and trailing return type.

The CC should state instead: Use const auto& or auto&& in range based for loops and algo-lambda-parameter or write a comment why not.

const int i = std::reduce(vec.begin(), vec.end(), [](const auto& l, const auto& r){return l + r;});

l and r is what i mean by algo-lambda-paramerer.

 

algorithm

There is already:

Quote

Use STL when appropriate.

But STL is an ambiguous term and this paragraph does not appreciate std::algorithms enought

Quote

Use "for range" loop instead of "std::for_each".

This general rule is not good. Because sometimes i have iterators or the body is an existingt function. In both situations it is bether to use std::for_each.

I would also write a paragraph about avoid old stile for-loops because iterations could be bypassed from within the body:

// avoid
for (size_t i = 0; i != 9; ++i)
{
	// The iterator can be mutated. This loop never ends.
	++i;
}
// saver
for (const size_t& i : std::ranges::iota(0, 9))
{
	++i; // compile-error 
}

 

Misc

• could we try to integrate the code from source/lib? Licence; brace style; string type.
  • propably rewrite it in source/ps.
• Own string type is a bad solution to add more functionality: free functions do it as well.

[vladislavbelov]

1 hour ago, phosit said:

This paragraph should be removed/shorted, because

• sometimes it does make the code more verbose by adding very little value,
• sometimes it is slower because of implicit conversion,
• and sometimes it is impossible lambda; structured binding and trailing return type.

The CC should state instead: Use const auto& or auto&& in range based for loops and algo-lambda-parameter or write a comment why not.

auto indeed saves some visual space, but at the same time in some cases it's less clear and isn't safe. Unfortunately we can't define a set of types that an auto might describe (as in more functional languages), auto is kind of workaround because C++ can't express some things better than with auto, fortunately there are concepts in C++20. C++ compiler verifies auto by used operations not by their semantic meanings. It means in your example we could pass a vector of strings to the reduce function and get a lot of string copies (accounting we don't have const int at the left).

I'd like to have something like that: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es11-use-auto-to-avoid-redundant-repetition-of-type-names . And addition or reinterpretation: "Use auto when a type class can be deduced by a reader without much context".

2 hours ago, phosit said:

But STL is an ambiguous term and this paragraph does not appreciate std::algorithms enought

Mostly it means prefer a STL function over the same function in a third party library or written by yourself.

2 hours ago, phosit said:

could we try to integrate the code from source/lib? Licence; brace style; string type.

• propably rewrite it in source/ps.

I'd like to remove the lib and merge its functionality into regular folder as well.

2 hours ago, phosit said:

Own string type is a bad solution to add more functionality: free functions do it as well.

Generally agree though the code might be longer because of that. F.e. having a FromDouble function in a global namespace isn't a good thing.

[vladislavbelov]

3 hours ago, phosit said:

This general rule is not good. Because sometimes i have iterators or the body is an existingt function. In both situations it is bether to use std::for_each.

I would also write a paragraph about avoid old stile for-loops because iterations could be bypassed from within the body:

// avoid
for (size_t i = 0; i != 9; ++i)
{
	// The iterator can be mutated. This loop never ends.
	++i;
}
// saver
for (const size_t& i : std::ranges::iota(0, 9))
{
	++i; // compile-error 
}

It's good if used with ranges. It's worth to mention though that MSVC sometimes might not inline lambdas and it might affect performance.

[Stan`]

Cc @hyperion, @Mercury, @jprahman

[hyperion]

Changing CC always has it's price and shouldn't be done lightly. The c++ core guidelines linked by @vladislavbelov earlier would otherwise be an ideal basis to start with as a parent for CC.

auto:

•   I like where the CC speaks of using auto in moderation.

old style for loops:

• well, one could theoretically argue anyone with any coding experience in any language knows how they work at a glance which might be of minor help in maintenance but personally I don't mind.

misc:

• moving lib/* to ps/* is fine with me, this is a historic curiosity that could stay but if someone wants to do all the cleanup, review, testing and committing - sure. The alternative is to split lib/* into an own repo and depend on it, so as to align with the original intent but I see this as the lesser approach here.
• I also have a preference for free functions over the CStr approach.

[phosit]

A link to CppCoreGuidelines would indeed be usefull.

14 hours ago, vladislavbelov said:

Having a FromDouble function in a global namespace isn't a good thing.

Yes but why? Would you go for str::FromDouble?

 

The misc part will not go in to CC. "use std::string were possible" would be to harsh.

[Stan`]

56 minutes ago, phosit said:

Yes but why? Would you go for str::FromDouble?

 

Xml parsing ?

[vladislavbelov]

1 hour ago, phosit said:

Yes but why? Would you go for str::FromDouble?

I was explaining why moving the functions out might increase the code length.

[hyperion]

4 hours ago, phosit said:

Yes but why? Would you go for str::FromDouble?

c-style ps_stringutils_stringFromDouble

as for str, I don't like this as a namespace name, I'd prefer a proper name. If it's to long in certain TU due to heavy use just use using or alias.

[phosit]

I made changes to CC: http://trac.wildfiregames.com/wiki/Coding_Conventions?version=56

The part about C++ Core Guidelines and that about algorithms are not as present as i wish them to be. But i didn't know what to write

[phosit]

The changes are reverted: Not all were discussed in this thread and I made some bad examples. The diff is attached

To explain all of them:

• Were to Include third party libraries was not defined in the CC. I formulated wat Vladislav said on IRC. The example with boost/optional is bad since we try to have few dependencyes on boost.
• I replaced "Use STL when appropriate." with a link to C++ Core Guidelines rule.
• At "Use nullptr" I added "This convention should be implicit but stated hear because there are still many places were `NULL` is used."
• I removed "Don't do `if(p) delete p;`". I havent seen this pattern in code and somebody knowing C++ would not write that -> will be pointed out at code-review
• I replaced the paragraph about auto with a link to C++ Core Guidelines and the sentence "Be aware that overuse of `auto` can also cause problems in the long-run."
• I moved the Code And Memory Performance guidelines to the bottom and added a link to C++ Core Guidelines.

Algorithms

I didnt wan't to write a harsh rule("No raw loop") or a wage one("Use algorithms when appropriate"). So wat i did was write a reason why not to use raw loops. here again the excample was bad since I wrote std::ranges::iota instead of std::views::iota and we don't require support for either.

CC.diff

[hyperion]

Quick notes only

header sorting:

• changing sort order might cause changes. IIRC curl on at least on windows isn't safe and we can't assume the code base to be free of "order hacks"

loops:

• std::ranges::iota is c++23