| Summary: | [CMake] Unify coding style for CMake files | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Halton Huo <halton.huo> | ||||||||||||||
| Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | benjamin, gyuyoung.kim, laszlo.gombos, lucas.de.marchi, morrita, mxie, ojan, paroga, rakuco, rwlbuis, webkit.review.bot | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 176659 [details]
Patch
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 ? Thanks for the patch! This is a great start. CMake accepts both ENDIF () and ENDIF (<condition>). Should we unify this as well? (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. (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. 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. (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? (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). (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. (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. Created attachment 177221 [details]
Patch
(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 ? Created attachment 177393 [details]
Patch
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.
(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. 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. (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. Created attachment 177629 [details]
Patch
(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 ! Created attachment 177653 [details]
Patch
(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. 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. Created attachment 177893 [details]
Patch
(In reply to comment #23) > Created an attachment (id=177893) [details] > Patch Rebase to trunk@136783 and address Laszlo's #22 comments. Comment on attachment 177893 [details]
Patch
r=me. Looks good to me.
Comment on attachment 177893 [details] Patch Clearing flags on attachment: 177893 Committed r136790: <http://trac.webkit.org/changeset/136790> All reviewed patches have been landed. Closing bug. |
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 ()