[Webkit-unassigned] [Bug 104240] [CMake] Add CMake style checker

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 6 07:03:45 PST 2012


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





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

I think the style checker would need some unittests just like the existing checkers. 

It would be great if someone else with more experience in the style checkers could help the review. Overall the direction looks good to me.

> Tools/Scripts/webkitpy/style/checkers/cmake.py:31
> +

Extra line ?

> Tools/Scripts/webkitpy/style/checkers/cmake.py:41
> +        self._no_space_cmds = self._read_file_ignore_comments(
> +                  os.path.join(basedir, "cmake-commands-no-space.txt"))
> +        self._one_space_cmds = self._read_file_ignore_comments(
> +                  os.path.join(basedir, "cmake-commands-one-space.txt"))

I think these lists should be in the cmake.py file as that is more consistent with the existent checkers.

> Tools/Scripts/webkitpy/style/checkers/cmake.py:55
> +            self._handle_style_error(line_number, 'whitespace/tailing', 5,
> +                                     'No tailing spaces')

tailing -> trailing

> Tools/Scripts/webkitpy/style/checkers/cmake.py:86
> +            if re.search('(^|\ +)' + t.upper() + '\ *\(', line_content):
> +                msg = 'Use lowercase command "' + t + '" instead of "' + t.upper() + '"'

What if someone would use mixed-case - e.g. "If" ? It seems to me that this rule would not catch it.

> Tools/Scripts/webkitpy/style/checkers/cmake.py:88
> +            if re.search('(^|\ +)' + t + '\ +\(', line_content):

Wouldn't t.lower() be better here ?

> Tools/Scripts/webkitpy/style/checkers/cmake.py:96
> +            if re.search('(^|\ +)' + t.upper() + '\ *\(', line_content):
> +                msg = 'Use lowercase command "' + t + '" instead of "' + t.upper() + '"'

Ditto - What if someone would use mixed-case - e.g. "If" ? It seems to me that this rule would not catch it.

Can this rule somehow shared, instead of repeated ?

> Tools/Scripts/webkitpy/style/checkers/cmake.py:98
> +            if re.search('(^|\ +)' + t + '(\(|\ \ +\()', line_content):

Ditto - Wouldn't t.lower() be better here ?

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


More information about the webkit-unassigned mailing list