RESOLVED FIXED 103605
[CMake] Unify coding style for CMake files
https://bugs.webkit.org/show_bug.cgi?id=103605
Summary [CMake] Unify coding style for CMake files
Halton Huo
Reported 2012-11-28 23:38:05 PST
The coding style in cmake files under Source/cmake/ are different, for example, indent mix used 2-space 4-space. After I check existing cmake files, most cmake files are following: * 4-space as indent, no TAB * Uppercase and no space when call macros: MESSAGE("testing") * 1-space after condition clause: IF (), ELSE (), ENDIf(), FOREACH () * 1-space after macro definition: MACRO (), ENDMACRO ()
Attachments
Patch (21.25 KB, patch)
2012-11-28 23:44 PST, Halton Huo
no flags
Patch (340.17 KB, patch)
2012-12-03 03:26 PST, Halton Huo
no flags
Patch (341.45 KB, patch)
2012-12-03 19:11 PST, Halton Huo
no flags
Patch (307.99 KB, patch)
2012-12-04 18:01 PST, Halton Huo
no flags
Patch (307.96 KB, patch)
2012-12-04 19:46 PST, Halton Huo
no flags
Patch (308.48 KB, patch)
2012-12-05 18:03 PST, Halton Huo
no flags
Halton Huo
Comment 1 2012-11-28 23:44:06 PST
Laszlo Gombos
Comment 2 2012-11-29 19:29:33 PST
Comment on attachment 176659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176659&action=review Thank you for the initiative. I do not think anyone would object trying to unify the style, but there needs to be an agreement on what the style is. I CCd a few more folks who might have input to the style. r- for now as at least the ChangeLog needs to be fixed, but more importantly I'd like to hear from other stakeholders before this lands. > ChangeLog:3 > + [EFL] uniformed coding style in cmake files If the intent is to change all files that are input to CMake you should change the prefix of the bug to [CMake]. We usually start the title with a capital letter; "Unify coding style for CMake files" sounds better to me for a title. > ChangeLog:8 > + Uniform coding style for .cmake files with rules: What about other files that CMake takes as input - e.g. CMakeLists.txt ? > ChangeLog:12 > + - 4-space as indent, no TAB > + - Uppercase and no space when call macros: MESSAGE("testing") > + - 1-space after condition clause: IF (), ELSE (), ENDIf(), FOREACH () > + - 1-space after macro definition: MACRO (), ENDMACRO () Where are all these rules coming from ? Are these custom for other projects as well ? Have you looked at other styles, like http://techbase.kde.org/Policies/CMake_Coding_Style (i just picked one, it does not mean that that is the one we should take as a base). > ChangeLog:19 > + * Source/cmake/WebKitPackaging.cmake: Again, what about CMakeLists.txt ?
Ming Xie
Comment 3 2012-11-29 20:02:05 PST
Thanks for the patch! This is a great start. CMake accepts both ENDIF () and ENDIF (<condition>). Should we unify this as well?
Gyuyoung Kim
Comment 4 2012-11-29 20:12:05 PST
(In reply to comment #2) > (From update of attachment 176659 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176659&action=review > > Thank you for the initiative. I do not think anyone would object trying to unify the style, but there needs to be an agreement on what the style is. I CCd a few more folks who might have input to the style. AFAIK, WebKit doesn't like to fix only coding style. However, I also don't wanna object to land this if all cmake developers like this patch.
Hajime Morrita
Comment 5 2012-11-29 22:59:11 PST
(In reply to comment #4) > AFAIK, WebKit doesn't like to fix only coding style. However, I also don't wanna object to land this if all cmake developers like this patch. Or, you can land for EFL part and ask other port folks to take their part. For CMake files, people who don't use it won't care about it.
Halton Huo
Comment 6 2012-11-29 23:44:34 PST
Thanks for all the comments, glad to know coding style change is welcomed. (In reply to comment #2) As Laszlo suggest, change the summary to "[CMake] Unify coding style for CMake files" > What about other files that CMake takes as input - e.g. CMakeLists.txt ? In this change I do not touch other cmake files like Findxxx.cmake and CMakeLists.txt. But I can include all of them, which is all .cmake and CMakeLists.txt files. But before that we need define a coding style for CMake files in WebKit project. > Where are all these rules coming from ? Are these custom for other projects as well ? Have you looked at other styles, like http://techbase.kde.org/Policies/CMake_Coding_Style (i just picked one, it does not mean that that is the one we should take as a base). As I said, I just use mostly used coding style that files under Source/cmake/. Referred to http://www.webkit.org/coding/coding-style.html, http://techbase.kde.org/Policies/CMake_Coding_Style and existing cmake 2.8 modules, I'd like to draft the coding style rules as following: 1. Indentation 1.1 Use spaces, not tabs(Indentation rule 1 in webkit C/C++ coding style) 1.2 4-space as indent(Indentation rule 2 in webkit C/C++ coding style) 2. Spacing 2.1 Place spaces between control statements and their parentheses. (Spacing rule 4 in webkit C/C++ coding style), IF (), ELSE (), ENDIf(), FOREACH () 2.2 Place spaces between function and macro statements and their parentheses. MACRO (), ENDMACRO (), FUNCTION (), ENDFUNCTION (). Same as 2.1 2.3 Do not place spaces between a function or macro and its parentheses, or between a parenthesis and its content. (Spacing rule 4 in webkit C/C++ coding style), MESSAGE("testing") not MESSAGE( "testing") or MESSAGE ("testing") 3. Uppercase when call macros and functions. ADD_EXECUTABLE() not add_executable(), SET() not set(). (Referred cmake 2.8 modules) Please let me know your comments and suggestions before I submit a new patch.
Patrick R. Gansterer
Comment 7 2012-11-30 00:46:14 PST
(In reply to comment #6) +1 for changing the style. > > Where are all these rules coming from ? Are these custom for other projects as well ? Have you looked at other styles, like http://techbase.kde.org/Policies/CMake_Coding_Style (i just picked one, it does not mean that that is the one we should take as a base). > > As I said, I just use mostly used coding style that files under Source/cmake/. Referred to http://www.webkit.org/coding/coding-style.html, http://techbase.kde.org/Policies/CMake_Coding_Style and existing cmake 2.8 modules, I'd like to draft the coding style rules as following: > 1. Indentation > 1.1 Use spaces, not tabs(Indentation rule 1 in webkit C/C++ coding style) > 1.2 4-space as indent(Indentation rule 2 in webkit C/C++ coding style) Agree. > 2. Spacing > 2.1 Place spaces between control statements and their parentheses. (Spacing rule 4 in webkit C/C++ coding style), IF (), ELSE (), ENDIf(), FOREACH () Agree. > 2.2 Place spaces between function and macro statements and their parentheses. MACRO (), ENDMACRO (), FUNCTION (), ENDFUNCTION (). Same as 2.1 I have no strong opinion on this, but if we say that a macro/function is something like a macro/function/method in C/C++, I think that no space would meet the WebKit C/C++ coding style more. > 2.3 Do not place spaces between a function or macro and its parentheses, or between a parenthesis and its content. (Spacing rule 4 in webkit C/C++ coding style), MESSAGE("testing") not MESSAGE( "testing") or MESSAGE ("testing") Agree. > 3. Uppercase when call macros and functions. ADD_EXECUTABLE() not add_executable(), SET() not set(). (Referred cmake 2.8 modules) AFAIK all lowercase is the "new" CMake style. If you look at new code added to the CMake repository, it is usually lowercase. Also the documentation at http://www.cmake.org/cmake/help/v2.8.10/cmake.html uses lowercase. Any ideas about adding a CMake codeing style checker? Or at least a well know place to put this new rules, so we can refer to it, when seeing violations?
Halton Huo
Comment 8 2012-11-30 01:01:41 PST
(In reply to comment #7) > > 2.2 Place spaces between function and macro statements and their parentheses. MACRO (), ENDMACRO (), FUNCTION (), ENDFUNCTION (). Same as 2.1 > I have no strong opinion on this, but if we say that a macro/function is something like a macro/function/method in C/C++, I think that no space would meet the WebKit C/C++ coding style more. Okay for me, +1. > > 3. Uppercase when call macros and functions. ADD_EXECUTABLE() not add_executable(), SET() not set(). (Referred cmake 2.8 modules) > AFAIK all lowercase is the "new" CMake style. If you look at new code added to the CMake repository, it is usually lowercase. Also the documentation at http://www.cmake.org/cmake/help/v2.8.10/cmake.html uses lowercase. Okay for me, +1. > Any ideas about adding a CMake codeing style checker? Or at least a well know place to put this new rules, so we can refer to it, when seeing violations? Good suggestion and I think the checker should do inside Tools/Script. Can I do that in a separately? As for the document, any possible to add CMake part under http://www.webkit.org/coding/coding-style.html? (I do not think I have permission to do that).
Patrick R. Gansterer
Comment 9 2012-11-30 01:17:17 PST
(In reply to comment #8) > (In reply to comment #7) > > > 2.2 Place spaces between function and macro statements and their parentheses. MACRO (), ENDMACRO (), FUNCTION (), ENDFUNCTION (). Same as 2.1 > > I have no strong opinion on this, but if we say that a macro/function is something like a macro/function/method in C/C++, I think that no space would meet the WebKit C/C++ coding style more. > Okay for me, +1. > > > > 3. Uppercase when call macros and functions. ADD_EXECUTABLE() not add_executable(), SET() not set(). (Referred cmake 2.8 modules) > > AFAIK all lowercase is the "new" CMake style. If you look at new code added to the CMake repository, it is usually lowercase. Also the documentation at http://www.cmake.org/cmake/help/v2.8.10/cmake.html uses lowercase. > Okay for me, +1. > > > Any ideas about adding a CMake codeing style checker? Or at least a well know place to put this new rules, so we can refer to it, when seeing violations? > Good suggestion and I think the checker should do inside Tools/Script. Can I do that in a separately? Of course! > As for the document, any possible to add CMake part under http://www.webkit.org/coding/coding-style.html? (I do not think I have permission to do that). The websites live at http://trac.webkit.org/browser/trunk/Websites, but I'm not sure if it's the correct place, since there are e.g. no JavaScript rules too.
Laszlo Gombos
Comment 10 2012-11-30 12:37:12 PST
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > > 2.2 Place spaces between function and macro statements and their parentheses. MACRO (), ENDMACRO (), FUNCTION (), ENDFUNCTION (). Same as 2.1 > > > I have no strong opinion on this, but if we say that a macro/function is something like a macro/function/method in C/C++, I think that no space would meet the WebKit C/C++ coding style more. > > Okay for me, +1. > > > > > > 3. Uppercase when call macros and functions. ADD_EXECUTABLE() not add_executable(), SET() not set(). (Referred cmake 2.8 modules) > > > AFAIK all lowercase is the "new" CMake style. If you look at new code added to the CMake repository, it is usually lowercase. Also the documentation at http://www.cmake.org/cmake/help/v2.8.10/cmake.html uses lowercase. > > Okay for me, +1. > > > > > Any ideas about adding a CMake codeing style checker? Or at least a well know place to put this new rules, so we can refer to it, when seeing violations? > > Good suggestion and I think the checker should do inside Tools/Script. Can I do that in a separately? > Of course! > > > As for the document, any possible to add CMake part under http://www.webkit.org/coding/coding-style.html? (I do not think I have permission to do that). > The websites live at http://trac.webkit.org/browser/trunk/Websites, but I'm not sure if it's the correct place, since there are e.g. no JavaScript rules too. These all sound great to me as well.
Halton Huo
Comment 11 2012-12-03 03:26:10 PST
Gyuyoung Kim
Comment 12 2012-12-03 03:32:25 PST
(In reply to comment #9) > > As for the document, any possible to add CMake part under http://www.webkit.org/coding/coding-style.html? (I do not think I have permission to do that). > The websites live at http://trac.webkit.org/browser/trunk/Websites, but I'm not sure if it's the correct place, since there are e.g. no JavaScript rules too. In EFL port case, we've maintained a wiki page for EFL coding rule. http://trac.webkit.org/wiki/EFLWebKitCodingStyle How about make a wiki page for cmake coding rule ?
Halton Huo
Comment 13 2012-12-03 19:11:21 PST
Halton Huo
Comment 14 2012-12-03 19:15:36 PST
Comment on attachment 177393 [details] Patch Expect some indent changes, all other changes to CMakeLists.txt and *.cmake are updated by Tools/Script/update-cmake-style. I leave a TODO in that script. I will use that script when check-webkit-style and webkit-path.
Halton Huo
Comment 15 2012-12-03 19:21:48 PST
(In reply to comment #12) > In EFL port case, we've maintained a wiki page for EFL coding rule. > http://trac.webkit.org/wiki/EFLWebKitCodingStyle > > How about make a wiki page for cmake coding rule ? Gyuyoung, done. Since the CMake rules are simple, I did not add example, just rules.
Laszlo Gombos
Comment 16 2012-12-04 12:06:10 PST
Comment on attachment 177393 [details] Patch 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.
Halton Huo
Comment 17 2012-12-04 17:57:27 PST
(In reply to comment #16) > 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. Refer to cmake doc (http://www.cmake.org/cmake/help/v2.8.10/cmake.html), the boolean expressions(AND, OR, NOT) are uppercase. Same as other keywords like APPEND, TARGETS, INSTALL are also uppercase. With my testing, they must be uppercase, otherwise cmake report error. The following example will fails: if (1 and 0) #CMake error "Unknown arguments specified" list(append SOURCES #CMake error "list does not recognize sub-command append" file1.cpp) endif () So I'd like to keep them uppercase and won't check the style for those because developer will meet CMake failure as above. I'll remake the patch with removing changes for ThirdParty and ignore my newly add script.
Halton Huo
Comment 18 2012-12-04 18:01:11 PST
Laszlo Gombos
Comment 19 2012-12-04 18:48:56 PST
(In reply to comment #17) > (In reply to comment #16) > > 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. > Refer to cmake doc (http://www.cmake.org/cmake/help/v2.8.10/cmake.html), the boolean expressions(AND, OR, NOT) are uppercase. Same as other keywords like APPEND, TARGETS, INSTALL are also uppercase. You are absolutely right, it is my bad. Thanks for checking. > I'll remake the patch with removing changes for ThirdParty and ignore my newly add script. Great, thanks !
Halton Huo
Comment 20 2012-12-04 19:46:56 PST
Halton Huo
Comment 21 2012-12-04 19:48:38 PST
(In reply to comment #20) > Created an attachment (id=177653) [details] > Patch Rebase to trunk@136609 and fix an error that WEBKIT_WRAP_SOURCELIST( is wrongly replaced as WEBKIT_WRAP_SOURCElist(, please review it again.
Laszlo Gombos
Comment 22 2012-12-05 10:43:18 PST
Comment on attachment 177653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177653&action=review Looks great, but would be great to fix the spelling in the ChangeLogs. > Source/JavaScriptCore/ChangeLog:22 > + 2.4 No space at line endding. endding -> ending. > Source/WTF/ChangeLog:22 > + 2.4 No space at line endding. Ditto; endding -> ending. > Source/WebCore/ChangeLog:22 > + 2.4 No space at line endding. Ditto; endding -> ending. > Source/WebKit/ChangeLog:22 > + 2.4 No space at line endding. Ditto; endding -> ending. > Source/WebKit/efl/ChangeLog:22 > + 2.4 No space at line endding. Ditto; endding -> ending. > Source/WebKit2/ChangeLog:22 > + 2.4 No space at line endding. Ditto; endding -> ending. > ChangeLog:22 > + 2.4 No space at line endding. Ditto; endding -> ending.
Halton Huo
Comment 23 2012-12-05 18:03:32 PST
Halton Huo
Comment 24 2012-12-05 18:05:14 PST
(In reply to comment #23) > Created an attachment (id=177893) [details] > Patch Rebase to trunk@136783 and address Laszlo's #22 comments.
Laszlo Gombos
Comment 25 2012-12-05 18:30:47 PST
Comment on attachment 177893 [details] Patch r=me. Looks good to me.
WebKit Review Bot
Comment 26 2012-12-05 19:11:10 PST
Comment on attachment 177893 [details] Patch Clearing flags on attachment: 177893 Committed r136790: <http://trac.webkit.org/changeset/136790>
WebKit Review Bot
Comment 27 2012-12-05 19:11:16 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.