[cmake-developers] Questions about coding conventions

[Brad King]

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.

> 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.

> 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.

> `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.

> 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.

-Brad