[Webkit-unassigned] [Bug 103605] [CMake] Unify coding style for CMake files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=103605


Laszlo Gombos <laszlo.gombos at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #177393|review?                     |review-
               Flag|                            |




--- Comment #16 from Laszlo Gombos <laszlo.gombos at webkit.org>  2012-12-04 12:06:10 PST ---
(From update of attachment 177393)
View in context: https://bugs.webkit.org/attachment.cgi?id=177393&action=review

Looks great overall, but r- for the changes under Source/ThirdParty. Feel free to ping me or other on irc for a quick review as this patch will rot fast.

> Source/ThirdParty/ChangeLog:26
> +        * gtest/CMakeLists.txt:

We should not change files under Source/ThirdParty. Files under Source/ThirdParty are usually exempt from style rules and we should try to avoid forking these files from upstream as much as possible.

> Source/ThirdParty/gtest/CMakeLists.txt:17
> -  include(CMakeForceCompiler)
> -  cmake_force_c_compiler("${gtest_compiler}" "")
> -  cmake_force_cxx_compiler("${gtest_compiler}" "")
> -endif()
> +    include(CMakeForceCompiler)
> +    cmake_force_c_compiler("${gtest_compiler}" "")
> +    cmake_force_cxx_compiler("${gtest_compiler}" "")
> +endif ()

Ditto. I do not think we should touch files Source/ThirdParty/.

> Source/WTF/wtf/CMakeLists.txt:228
>          TCSystemAlloc.cpp

Nit: Having NOT (AND, etc) and APPEND (INSTERT, etc) also all lowercase would be more consistent and it would be easier to read. If this make sense, it could be in a follow-up patch.

> Tools/Scripts/update-cmake-style:32
> +update-cmake-style [-d][-b][-v] [ file ... ]

Thank you for sharing this script. It looks great but I am not sure if we should land this in the source tree as I think this is mainly for one time use once we start enforcing the style. Instead it would be great to have a style checker script for cmake. You can find the existing checker scripts under Tools/Scripts/webkitpy/style/checkers. You might be able to reusing some of your code for the style checker.

I think the tooling work can be part of a separate patch as this patch is already touching on many files that change frequently with likely merge conflicts.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.