[cmake-developers] Questions about coding conventions

[Daniel Pfeifer]

On Fri, Jun 10, 2016 at 3:43 PM, Brad King <brad.king at kitware.com> wrote:
> On 06/10/2016 09:30 AM, Daniel Pfeifer wrote:
>> By looking at the CMake source code, there are some inconsistencies
>> regarding coding conventions. This is not a big problem and fixing
>> them probably does not have a high priority.
>> I would like to know what conventions to follow for new code.
>
> Please look at documenting this in CONTRIBUTING.rst once we resolve
> this discussion.

ok.

>> Formatting: No longer an issue. A .clang-format is provided and most
>> inconsistencies have disappeared.
>
> Yes, and thanks for all your work on that.
>
>> Braces around single conditional statements: This is currently not
>> consistent. Should they be avoided? Should they be required? We could
>> add the missing ones with clang-tidy. If we want them.
>
> The intention has been to use braces for all blocks even with single
> statements.  Due to lack of documentation and enforcement this has
> not been maintained consistently.

I have added some missing ones:
https://cmake.org/gitweb?p=stage/cmake.git;a=commit;h=a16bf141bc5d393b27d13d8235d95a1b034052c2

>> Naming conventions: Classes are named  cmLikeThis. Member functions
>> and member variables are named LikeThis. Local variables are named
>> likeThis. Members are always accessed with `this->`.
>
> Yes, though there are also many local variables named `like_this` too.
> I have no strong feelings on enforcing local variable name conventions.
>
>> So far it is pretty consistent. But how to name free functions and
>> macros? I have seen all kinds of variations.
>
> No convention was ever established for those.

I see. Any strong opinion whether a convention should be established?
I would assume UPPER_CASE for macros. Any prefix?
Any opinions about free functions?

>> `const T` or `T const` (also: `const T&` or `T const&`): Currently,
>> CMake uses both. I have not analyzed which one dominates.
>
> I prefer `T const` always, except for `const char*` specifically.

Good.

>> Passing and returning strings: We have `const char*`, `std::string`,
>> and `std::string const&`. Unifying this can affect performance.
>> Storing `const char*` in a `std::string` creates a (potentially
>> unneded) copy (assuming it is not null). `const char*` can distinguish
>> invalid from empty. Do we actually need this distinction?
>
> Last year we had a few cleanup passes done along those lines, but
> it is far from consistent everywhere.  There  are cases where we
> need to distinguish invalid from empty, like GetDefinition().  It
> would be nice to have an optional<string> or something like that
> so we can keep the distinction but avoid using `const char*` all
> over.  Any change like this should be introduced gradually, as
> many of the related areas are performance sensitive.

So the convention would be to use `std::string` or
`cmOptional<std::string>` as return values. Should `std::string
const&` be allowed as return value?
Arguments should be preferred as `const&` I assume.


Some other questions:

Can't `std::ifstream` and `std::ofstream` be used directly? It seams
that kwsys does some workarounds for Visual Studio 2005 and newer.
That surprises me. Workarounds are usually used for older, not newer
compilers.

I see that `#include <string.h>` is preferred over `#include
<cstring>`. Are there old compilers supported that prohibit the latter
or was it just not changed yet?

cmStandardIncludes.h includes many standard headers. That is against
the idea of include-what-you-use. Is there a particular requirement or
was it just not cleaned up yet?

Should we begin using newer language features optionally by providing
macros like CM_OVERRIDE?