[webkit-reviews] review denied: [Bug 103605] [CMake] Unify coding style for CMake files : [Attachment 177393] Patch

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.gombos at webkit.org> has denied Halton Huo
<halton.huo at gmail.com>'s request for review:
Bug 103605: [CMake] Unify coding style for CMake files
https://bugs.webkit.org/show_bug.cgi?id=103605

Attachment 177393: Patch
https://bugs.webkit.org/attachment.cgi?id=177393&action=review

------- Additional Comments from Laszlo Gombos <laszlo.gombos at webkit.org>
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.