Re: [cmake-developers] Code style auto-formatting
On Mon, 2016-05-09 at 14:17 -0400, Brad King wrote: > > What is the rationale for "ColumnLimit: 79"? To make a line fit on old > > school terminals, 80 should be OK. To make a diff or an email response > > fit on old school terminals, two extra spaces are reserved. That leads > > to a column limit of 78. Where does 79 come from? > > That has been CMake's limit for a long time. Since we're changing the > style anyway we might as well go with the more common 80 chars that is > default in the Mozilla style. Not to say whether or not this should be changed, but FYI the 79 comes from this set of facts: * Traditional terminals are 80 chars wide * Editors and viewers that do line-wrapping use the last character of the line to specify whether there was a wrap, otherwise you can't tell if the line is wrapped or there's a newline. So a line that is exactly 80 characters long would look like this in such a viewer: this line...is 80 chars lon\ g and this isn't Hence the restriction to 79 chars to avoid that wrap. Also, some kind -of-broken terminals used to automatically scroll the window if you wrote a character to the bottom-right-most character (line 24 / char 80). -- 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
Re: [cmake-developers] Code style auto-formatting
On 05/09/2016 01:48 PM, Daniel Pfeifer wrote: > "AllowShortFunctionsOnASingleLine: Inline" is default in the Mozilla > preset. This line can be removed. Yes, thanks. > What is your rationale for "IndentCaseLabels: false"? I find that > indenting them increases readability when switch statements use > hanging braces. If the opening "{" is on a separate line, indenting > the labels does not improve readability that much. I experimented with it and liked the reduced horizontal space usage, especially given the column limit. However I see you're right about this case: switch (x) { case 1: { } break; default: { } } versus switch (x) { case 1: { } break; default: { } } We can drop that line. > What is the rationale for "ColumnLimit: 79"? To make a line fit on old > school terminals, 80 should be OK. To make a diff or an email response > fit on old school terminals, two extra spaces are reserved. That leads > to a column limit of 78. Where does 79 come from? That has been CMake's limit for a long time. Since we're changing the style anyway we might as well go with the more common 80 chars that is default in the Mozilla style. This brings us to: --- # This configuration requires clang-format 3.8 or higher. BasedOnStyle: Mozilla AlignOperands: false AlwaysBreakAfterReturnType: None AlwaysBreakAfterDefinitionReturnType: None Standard: Cpp03 ... -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
Re: [cmake-developers] Code style auto-formatting
On Mon, May 9, 2016 at 11:14 AM, Brad King wrote: > On 05/02/2016 10:08 AM, Brad King wrote: >> Next I'll look at the style updates themselves. > > I've made some more preparatory commits: > > Isolate formatted streaming blocks with clang-format off/on > https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=64b55203 > > Move comments off of class access specifier lines > https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=85425a3e > > Help clang-format wrap after braces on long initializer lists > https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=afca3735 > > Remove `//--...` horizontal separator comments > https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=0ac18d40 > > These changes improve the formatting output in some local > experiments. > > With the earlier changes to the #include order we no longer need > to use any custom `IncludeCategories`. > > Here is the `.clang-format` file I'd like to use: > > --- > # This configuration requires clang-format 3.8 or higher. > BasedOnStyle: Mozilla > AlignOperands: false > AllowShortFunctionsOnASingleLine: Inline > AlwaysBreakAfterReturnType: None > AlwaysBreakAfterDefinitionReturnType: None > ColumnLimit: 79 > IndentCaseLabels: false > Standard: Cpp03 > ... > "AllowShortFunctionsOnASingleLine: Inline" is default in the Mozilla preset. This line can be removed. What is your rationale for "IndentCaseLabels: false"? I find that indenting them increases readability when switch statements use hanging braces. If the opening "{" is on a separate line, indenting the labels does not improve readability that much. What is the rationale for "ColumnLimit: 79"? To make a line fit on old school terminals, 80 should be OK. To make a diff or an email response fit on old school terminals, two extra spaces are reserved. That leads to a column limit of 78. Where does 79 come from? -- 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
Re: [cmake-developers] Code style auto-formatting
On Mon, May 9, 2016 at 11:31 AM, Rolf Eike Beer wrote: > Am Montag, 9. Mai 2016, 13:14:17 schrieb Brad King: >> On 05/02/2016 10:08 AM, Brad King wrote: >> > Next I'll look at the style updates themselves. >> >> I've made some more preparatory commits: > > […] >> Remove `//--...` horizontal separator comments >> https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=0ac18d40 > > This one did not remove the additional newlines if there was a blank line > before and after the marker. This is not a problem. The empty lines will be removed by clang-format. -- 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
Re: [cmake-developers] Code style auto-formatting
Am Montag, 9. Mai 2016, 13:14:17 schrieb Brad King: > On 05/02/2016 10:08 AM, Brad King wrote: > > Next I'll look at the style updates themselves. > > I've made some more preparatory commits: […] > Remove `//--...` horizontal separator comments > https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=0ac18d40 This one did not remove the additional newlines if there was a blank line before and after the marker. Eike signature.asc Description: This is a digitally signed message part. -- 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
Re: [cmake-developers] Code style auto-formatting
On 05/02/2016 10:08 AM, Brad King wrote: > Next I'll look at the style updates themselves. I've made some more preparatory commits: Isolate formatted streaming blocks with clang-format off/on https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=64b55203 Move comments off of class access specifier lines https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=85425a3e Help clang-format wrap after braces on long initializer lists https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=afca3735 Remove `//--...` horizontal separator comments https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=0ac18d40 These changes improve the formatting output in some local experiments. With the earlier changes to the #include order we no longer need to use any custom `IncludeCategories`. Here is the `.clang-format` file I'd like to use: --- # This configuration requires clang-format 3.8 or higher. BasedOnStyle: Mozilla AlignOperands: false AllowShortFunctionsOnASingleLine: Inline AlwaysBreakAfterReturnType: None AlwaysBreakAfterDefinitionReturnType: None ColumnLimit: 79 IndentCaseLabels: false Standard: Cpp03 ... Plus the custom one for Cpp11 in Tests/CompileFeatures. The next few weeks will be a good time to do a one-shot style change commit because the feature freeze for the CMake 3.6 release is June 1. It is important to do sweeping changes like this shortly before a release branch is created because that way any fixes that must be backported to the branch do not conflict with the style change. Furthermore most fixes needed to the previous release have already been done. If we don't do it now we will have to wait about 4 months for the next stable period. If the above looks good I'll start an ANNOUNCE thread for this to draw more attention to it. While it will be possible to script style updates while rebasing topics across this change, it will be simpler if developers start new topics after the style change. -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
[cmake-developers] [CMake 0016097]: FindCUDA.cmake implicit target_link_libraries() can not be mixed with new signature
The following issue has been SUBMITTED. == https://cmake.org/Bug/view.php?id=16097 == Reported By:Nils Gladitz Assigned To: == Project:CMake Issue ID: 16097 Category: CMake Reproducibility:always Severity: major Priority: normal Status: new == Date Submitted: 2016-05-09 10:31 EDT Last Modified: 2016-05-09 10:31 EDT == Summary:FindCUDA.cmake implicit target_link_libraries() can not be mixed with new signature Description: e.g. CUDA_ADD_LIBRARY currently implicitly links to the CUDA libraries with: target_link_libraries(${cuda_target} ${CUDA_LIBRARIES}) Afterwards additional libraries can not be linked with the newer PRIVATE|PUBLIC|INTERFACE signature (since signatures can not be mixed). It would be nice if one of these keywords could be added to the implicit target_link_libraries() calls upon user request. Steps to Reproduce: cmake_minimum_required(VERSION 3.5.1) find_package(CUDA REQUIRED) file(WRITE my_cuda_lib.cpp "") cuda_add_library(my_cuda_lib SHARED my_cuda_lib.cpp) file(WRITE some_other_lib.cpp "") add_library(some_other_lib SHARED some_other_lib.cpp) # The plain signature for target_link_libraries has already been used with # the target "my_cuda_lib". All uses of target_link_libraries with a target # must be either all-keyword or all-plain. target_link_libraries(my_cuda_lib PRIVATE some_other_lib) == Issue History Date ModifiedUsername FieldChange == 2016-05-09 10:31 Nils Gladitz New Issue == -- 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
Re: [cmake-developers] proposal of fix for FindLua
On 05/06/2016 09:02 AM, ivan Ivanov wrote: > Fix for https://cmake.org/Bug/view.php?id=15756 Thanks for working on this. Please split the patch to first perform refactoring like moving the version extraction into a helper function. Then the actual logic change will be easier to see in the second commit. Also please name helper functions as "_lua_...". We might as well rename the set_lua_version_vars helper while we're at it. > +unset(LUA_INCLUDE_PREFIX CACHE) > +find_path(LUA_INCLUDE_PREFIX ${subdir}/lua.h This renames the cache entry from LUA_INCLUDE_DIR to LUA_INCLUDE_PREFIX and also stops re-using an already-found or user-provided value. Neither of these follows CMake conventions. I don't follow why this change is needed. Using version-specific paths in a normal find_path(LUA_INCLUDE_DIR ...) should be enough. Thanks, -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