Re: [cmake-developers] Code style auto-formatting

2016-05-09 Thread Paul Smith 
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

2016-05-09 Thread Brad King 
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

2016-05-09 Thread Daniel Pfeifer 
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

2016-05-09 Thread Daniel Pfeifer 
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

2016-05-09 Thread Rolf Eike Beer 
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

2016-05-09 Thread 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:

 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

2016-05-09 Thread Mantis Bug Tracker 

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

2016-05-09 Thread Brad King 
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