[webkit-reviews] review denied: [Bug 25884] WebKit needs a style linting tool : [Attachment 32509] Add cpplint w/ some autofix

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 25884: WebKit needs a style linting tool
https://bugs.webkit.org/show_bug.cgi?id=25884

Attachment 32509: Add cpplint w/ some autofix
https://bugs.webkit.org/attachment.cgi?id=32509&action=review

------- Additional Comments from David Levin <levin at chromium.org>
General comments:

Thanks for tackling this!

First, I know it was suggested that this tool be able to automatically fix
errors but this patch is huge without that support.  It would be nice to do the
review without that added support and then add it.  I have two reasons for
this:
   1. It is less to review in this initial pass.
   2. I would like to be able to review that code in more detail since it is
pretty new and untested (unlike the rest of the code which mostly has been in
use already).

Secondly, WebKit python code is following the python coding style
(http://www.python.org/dev/peps/pep-0008/, which is different from Google's). 
The main differences are 
   1. indent code by 4 space not 2. Fortunately, you can use
http://svn.python.org/projects/python/trunk/Tools/scripts/reindent.py to
automatically do this for you.
   2. Function/Method names should be lower_case_with_underscores as opposed to
CamelCase

I didn't do an in depth review at this point, but I left a few comments on
things I noticed during a quick look.

> +++ b/WebKitTools/Scripts/cpplint.py
> @@ -0,0 +1,3362 @@
> +#!/usr/bin/python2.4

"#!/usr/bin/python"


> +# Here are some issues that I've had people identify in my code during
reviews,
> +# that I think are possible to flag automatically in a lint tool.  If these
were
> +# caught by lint, it would save time both for myself and that of my
reviewers.
> +# Most likely, some of these are beyond the scope of the current lint
framework,
> +# but I think it is valuable to retain these wish-list items even if they
cannot
> +# be immediately implemented.
> +#
> +#  Suggestions
> +#  -----------
> +#  - Check for no 'explicit' for multi-arg ctor
> +#  - Check for boolean assign RHS in parens
> +#  - Check for ctor initializer-list colon position and spacing
> +#  - Check that if there's a ctor, there should be a dtor
> +#  - Check accessors that return non-pointer member variables are
> +#	declared const
> +#  - Check accessors that return non-const pointer member vars are
> +#	*not* declared const
> +#  - Check for using public includes for testing
> +#  - Check for spaces between brackets in one-line inline method
> +#  - Check for no assert()
> +#  - Check for spaces surrounding operators
> +#  - Check for 0 in pointer context (should be NULL)
> +#  - Check for 0 in char context (should be '\0')
> +#  - Check for camel-case method name conventions for methods
> +#	that are not simple inline getters and setters
> +#  - Check that base classes have virtual destructors
> +#	put "  // namespace" after } that closes a namespace, with
> +#	namespace's name after 'namespace' if it is named.
> +#  - Do not indent namespace contents
> +#  - Avoid inlining non-trivial constructors in header files
> +#  - Check for old-school (void) cast for call-sites of functions
> +#	ignored return value
> +#  - Check for class declaration order (typedefs, consts, enums,
> +#	ctor(s?), dtor, friend declarations, methods, member vars)

Several of these are either not relevant to WebKit or just plain not correct,
so I think this whole comment should just be deleted.




> +# Matches invalid increment: *count++, which moves pointer insead of

s/insead/instead/

> +
> +def CheckInvalidIncrement(filename, clean_lines, linenum, error):
> +  """Checks for invalud increment *count++.

s/invalud/invalid/