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 -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
