My plan in this bug is to port check-webkit-style into mozilla-central, add a mach target for it. Later a bugzilla robot can automatically run the mozilla-central tip revision of the tool for patches posted in bugzilla. Note that check-webkit-style runs a set of regex over the source file and if a warning matches a line modified by the patch it will report it.

I'm tentatively asking roc for review feel free to reassign to someone more appropriate. I split up the import/mozilla changes so we get a nice diff/history. I didn't know where to stick the code. I create tools/script/check-moz-style which is more nested then I need and a webkit idium but it creates a good place for other scripts to go into. Let me know if you have a better idea where this should go.

I did a quick audit of some code style checks. Since this will live in the mozilla tree it will be easy to later change our minds to tweak the rules if we hit too many false positive/negatives. We can also add some patterns particular to our code base and obsolete macros later.

I should note that I didn't replace most instance of Webkit in the script. Not sure what the proper policy is.

Comment on attachment 753579 [details] [diff] [review] Part 1: Import code Review of attachment 753579 [details] [diff] [review]: ----------------------------------------------------------------- Please add the revision from which you took this code to the commit message.

(In reply to Benoit Girard (:BenWa) from comment #5) > I should note that I didn't replace most instance of Webkit in the script. > Not sure what the proper policy is. That's fine.

Comment on attachment 753583 [details] [diff] [review] Part 2: Add HG support + update style checks Review of attachment 753583 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/scripts/check-moz-style/modules/cpplint.py @@ -3073,2 @@ > '-whitespace/blank_line', > - '-runtime/explicit', # explicit I'd argue that we should keep the explicit style check. @@ -3074,5 @@ > - '-runtime/explicit', # explicit > - '-runtime/virtual', # virtual dtor > - '-runtime/printf', > - '-runtime/threadsafe_fn', > - '-runtime/rtti', We definitely want the rtti check. @@ -3091,2 @@ > '-runtime/casting', > - '-runtime/sizeof', Not sure if we don't want the sizeof check. ::: tools/scripts/check-moz-style/modules/scm.py @@ +394,5 @@ > def files_changed_summary_for_commit(self, commit_id): > return self.run_command(['git', 'diff-tree', '--shortstat', '--no-commit-id', commit_id]) > + > + > +# All git-specific logic should go here. Nit: hg.

Thanks for the feedback ehsan: I made some progress on the robot. Still needs a few tweaks. I have no idea how it will deal with patches that don't directly apply to mozilla-central: https://landfill.bugzilla.org/bzapi_sandbox/show_bug.cgi?id=11613#c6

(In reply to comment #9) > Thanks for the feedback ehsan: > > I made some progress on the robot. Still needs a few tweaks. I have no idea how > it will deal with patches that don't directly apply to mozilla-central: > https://landfill.bugzilla.org/bzapi_sandbox/show_bug.cgi?id=11613#c6 It doesn't have to, it can just return an error.

> ::: tools/scripts/check-moz-style/modules/cpplint.py Is the new scripts subdirectory necessary? Most of the code in the tools directory could be considered "scripts". I would suggest a name like tools/lint/check-moz-style or just tools/check-moz-style.

Comment on attachment 753583 [details] [diff] [review] Part 2: Add HG support + update style checks Review of attachment 753583 [details] [diff] [review]: ----------------------------------------------------------------- These files also introduce some new trailing whitespace. A trailing whitespace check would be a useful addition to check-moz-style. :) ::: tools/scripts/check-moz-style/modules/cpplint.py @@ +2128,4 @@ > and not match(r'\s*\w+\s*:\s*$', cleansed_line)): > error(filename, line_number, 'whitespace/indent', 3, > 'Weird number of spaces at line-start. ' > + 'Are you using atleast 2-space indent?') s/atleast/at least/

(In reply to comment #12) > Comment on attachment 753583 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=753583 > Part 2: Add HG support + update style checks > > Review of attachment 753583 [details] [diff] [review]: > ----------------------------------------------------------------- > > These files also introduce some new trailing whitespace. A trailing whitespace > check would be a useful addition to check-moz-style. :) Oh, yes!!!

(In reply to Chris Peterson (:cpeterson) from comment #12) > Comment on attachment 753583 [details] [diff] [review] > Part 2: Add HG support + update style checks > > Review of attachment 753583 [details] [diff] [review]: > ----------------------------------------------------------------- > > These files also introduce some new trailing whitespace. A trailing > whitespace check would be a useful addition to check-moz-style. :) > It already supports it.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8) > Comment on attachment 753583 [details] [diff] [review] > Part 2: Add HG support + update style checks > > Review of attachment 753583 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: tools/scripts/check-moz-style/modules/cpplint.py > @@ -3073,2 @@ > > '-whitespace/blank_line', > > - '-runtime/explicit', # explicit > > I'd argue that we should keep the explicit style check. We are. There's a double negation. I'm removing the line that disabled it. > > @@ -3074,5 @@ > > - '-runtime/explicit', # explicit > > - '-runtime/virtual', # virtual dtor > > - '-runtime/printf', > > - '-runtime/threadsafe_fn', > > - '-runtime/rtti', > > We definitely want the rtti check. Double negation > > @@ -3091,2 @@ > > '-runtime/casting', > > - '-runtime/sizeof', > > Not sure if we don't want the sizeof check. Double negation Since you got the negation backwards are there any rules that I'm adding that you think should be disabled?

Had a bit of time on the plane, so I decided to write some tests an enforce the patch commit summary/commit message.

(In reply to Benoit Girard (:BenWa) from comment #15) > Since you got the negation backwards are there any rules that I'm adding > that you think should be disabled? Oops! Everything makes sense now. :-)

Created the robot repo: https://github.com/bgirard/PatchReviewRobot

I'm not a great person to review this since I don't know anything about writing good Python code. But I'll review the rules.

Actually to make life easier for everyone it would be great to have a summary checked in somewhere of the rules that are checked.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > Actually to make life easier for everyone it would be great to have a > summary checked in somewhere of the rules that are checked. The tests give a good approximation of that but a summary would be better.

You might want to try Ms2get or jhammel as they volunteered to review the original mach code, and we need to make sure that no good deed ever goes unpunished!

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #24) > You might want to try Ms2get or jhammel as they volunteered to review the > original mach code, and we need to make sure that no good deed ever goes > unpunished! Feel like reviewing this code ms2ger?

Comment on attachment 754174 [details] [diff] [review] Part 1: Import code Review of attachment 754174 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/check-moz-style/modules/cpplint.py @@ +553,5 @@ > + > + prefix = os.path.commonprefix([root_dir, project_dir]) > + return fullname[len(prefix) + 1:] > + > + # Not SVN? Try to find a git top level directory by This will need to support HG too, I guess. Actually, our requirement doesn't match theirs, so this code is wrong anyway. @@ +1589,5 @@ > + error(filename, line_number, 'whitespace/parens', 5, > + 'Mismatching spaces inside () in %s' % matched.group(1)) > + if not len(matched.group(2)) in [0, 1]: > + error(filename, line_number, 'whitespace/parens', 5, > + 'Should have zero or one spaces inside ( and ) in %s' % (We'll want to limit it to 0.) @@ +1836,5 @@ > + previous_line = clean_lines.elided[line_number - 2] > + if (previous_line.find('{') > 0 > + and search(r'\b(if|for|foreach|while|else)\b', previous_line)): > + error(filename, line_number, 'whitespace/braces', 4, > + 'One line control clauses should not use braces.') This is wrong; make sure it's disabled @@ +2055,5 @@ > + # See if NULL occurs in any comments in the line. If the search for NULL using the raw line > + # matches, then do the check with strings collapsed to avoid giving errors for > + # NULLs occurring in strings. > + if search(r'\bNULL\b', line) and search(r'\bNULL\b', CleansedLines.collapse_strings(line)): > + error(filename, line_number, 'readability/null', 4, 'Use 0 instead of NULL.') Make this nullptr instead of disabling the check? @@ +2154,5 @@ > + > + if cleansed_line.strip().endswith('||') or cleansed_line.strip().endswith('&&'): > + error(filename, line_number, 'whitespace/operators', 4, > + 'Boolean expressions that span multiple lines should have their ' > + 'operators on the left side of the line instead of the right side.') This is wrong, we use the opposite style. @@ +2396,5 @@ > + # I just try to capture the most common basic types, though there are more. > + # Parameterless conversion functions, such as bool(), are allowed as they are > + # probably a member operator declaration or default constructor. > + matched = search( > + r'\b(int|float|double|bool|char|int32|uint32|int64|uint64)\([^)]', line) Should have _t added.

Comment on attachment 754176 [details] [diff] [review] Part 2: Add HG support + update style checks Review of attachment 754176 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/check-moz-style/check-moz-style @@ -86,5 @@ > To see a list of all the categories used in %(program_name)s, pass no arg: > --filter= > """ % {'program_name': sys.argv[0]} > > - Wrong per pep8 @@ +121,5 @@ > > > def main(): > + global cwd > + global scm Those shouldn't be globals ::: tools/check-moz-style/modules/cpplint.py @@ +1814,1 @@ > if match(r'\s*{\s*$', line): What all is disabled here, and why? @@ +2123,5 @@ > if line and line[-1].isspace(): > error(filename, line_number, 'whitespace/end_of_line', 4, > 'Line ends in whitespace. Consider deleting these extra spaces.') > # There are certain situations we allow one space, notably for labels > + elif ((initial_spaces == 1) Might want to avoid 3-space too @@ +2422,5 @@ > # In addition, we look for people taking the address of a cast. This > # is dangerous -- casts can assign to temporaries, so the pointer doesn't > # point where you think. > + # XXX I think this warning is wrong. With multiple inheritance you NEED to > + # take the address after the cast. It seems correct to me. @@ -3073,3 @@ > '-whitespace/blank_line', > - '-runtime/explicit', # explicit > - '-runtime/virtual', # virtual dtor Leave this disabled; we already have build warnings for the issue, and this tool doesn't catch MOZ_FINAL. ::: tools/check-moz-style/modules/scm.py @@ +51,1 @@ > return None Don't leave code after a 'raise'. @@ +417,5 @@ > + > + def create_patch(self): > + if self.run_command(['hg', 'diff']) != "": > + sys.stderr.write("Warning: outstanding changes not include in style check.\n") > + return self.run_command(['hg', 'export', 'tip']) So we only support checking tip?

Comment on attachment 754888 [details] [diff] [review] Part 3: Check patch summary/description + Tests Review of attachment 754888 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/check-moz-style/checkmozstyle.py @@ +88,5 @@ > --filter= > """ % {'program_name': sys.argv[0]} > > +def match(pattern, string): > + return re.compile(pattern).match(string) I don't think this makes sense; you're compiling the regexp over and over again. We're probably fine with just using re.match; see <http://stackoverflow.com/questions/452104/is-it-worth-using-pythons-re-compile>. @@ +98,5 @@ > patch_string: A string of a patch. > """ > patch = DiffParser(patch_string.splitlines()) > > + if len(patch.files) == 0: if not patch.files: @@ +99,5 @@ > """ > patch = DiffParser(patch_string.splitlines()) > > + if len(patch.files) == 0: > + cpplint.error("patch", 0, "path/notempty", 3, "Patch does not appear to diff against any file.") "patch/", not "path/" throughout @@ +107,5 @@ > + cpplint.error("patch", 0, "path/nosummary", 3, "Patch does not have a summary.") > + else: > + proper_format = match(r"^Bug [0-9]+ - ", patch.status_line) > + if not proper_format: > + cpplint.error("patch", 0, "path/bugnumber", 3, "Patch summary should begin with 'Bug XXXXX - '.") Should probably allow "No bug - " @@ +108,5 @@ > + else: > + proper_format = match(r"^Bug [0-9]+ - ", patch.status_line) > + if not proper_format: > + cpplint.error("patch", 0, "path/bugnumber", 3, "Patch summary should begin with 'Bug XXXXX - '.") > + trailing ws @@ +111,5 @@ > + cpplint.error("patch", 0, "path/bugnumber", 3, "Patch summary should begin with 'Bug XXXXX - '.") > + > + > + if not patch.patch_description: > + cpplint.error("patch", 0, "path/nodescription", 3, "Patch does not have a description.") I'm not sure we want to require this. I know dbaron is a big fan, but my patches are usually small enough that an extra description is helpful. ::: tools/check-moz-style/modules/cpplint.py @@ +644,5 @@ > # the verbosity level isn't high enough, or the filters filter it out. > if _should_print_error(category, confidence): > _cpplint_state.increment_error_count() > if _cpplint_state.output_format == 'vs7': > + write_error('%s(%s): %s [%s] [%d]\n' % ( I'd prefer a separate patch doing the renaming @@ +3099,5 @@ > global _DEFAULT_FILTERS > _DEFAULT_FILTERS = [ > '-whitespace/comments-doublespace', > '-whitespace/blank_line', > + '-build/include', # Webkit specific I do want to add build/include_order (though it needs a fix for the config.h requirement). @@ +3108,5 @@ > '-build/storage_class', # const static > '-build/endif_comment', > '-whitespace/labels', > '-runtime/arrays', # variable length array > '-build/header_guard', We should add a variant of that; current implementation is stupid, though. @@ +3109,5 @@ > '-build/endif_comment', > '-whitespace/labels', > '-runtime/arrays', # variable length array > '-build/header_guard', > '-runtime/casting', Can we sort this list? ::: tools/check-moz-style/run_tests.py @@ +8,5 @@ > +import tempfile > +import checkmozstyle > +import subprocess > +import os > +import sys run_tests.py:7: 'filecmp' imported but unused run_tests.py:8: 'tempfile' imported but unused run_tests.py:10: 'subprocess' imported but unused run_tests.py:12: 'sys' imported but unused @@ +13,5 @@ > +import modules.cpplint as cpplint > + > +#filecmp.cmp('undoc.rst', 'undoc.rst') > +#tempFileTuple = tempfile.mkstemp() > +#os.write(tempFileTuple[0], "foo\n") Remove commented-out code @@ +18,5 @@ > + > +TESTS = [ > + # Empty patch > + { > + "patch" : "tests/test1.patch", No ws before the colons @@ +50,5 @@ > +] > + > +def main(): > + > + has_failed = False Follow pep8: Four-space indentation @@ +67,5 @@ > + > + test_status = "PASSED" > + if result != expected_output: > + test_status = "FAILED" > + print "TEST " + test["patch"] + " " + test_status New code should be py3-compatible: add 'from __future__ import print_function' at the top and add parens. I'd prefer "TEST %s %s" % (test["patch"], test_status) @@ +69,5 @@ > + if result != expected_output: > + test_status = "FAILED" > + print "TEST " + test["patch"] + " " + test_status > + print "Got result:\n" + result + "Expected:\n" + expected_output > + has_failed = True Unused @@ +70,5 @@ > + test_status = "FAILED" > + print "TEST " + test["patch"] + " " + test_status > + print "Got result:\n" + result + "Expected:\n" + expected_output > + has_failed = True > + else: Trailing ws

Comment on attachment 754176 [details] [diff] [review] Part 2: Add HG support + update style checks Review of attachment 754176 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/check-moz-style/modules/cpplint.py @@ +1814,1 @@ > if match(r'\s*{\s*$', line): The following warnings: * This { should be at the end of the previous line * Place brace on its own line for function definitions * One line control clauses should not use braces. I don't think these are appropriate for our style guideline. For example: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Control_Structures I added a comment and a TODO to split this off. @@ +2422,5 @@ > # In addition, we look for people taking the address of a cast. This > # is dangerous -- casts can assign to temporaries, so the pointer doesn't > # point where you think. > + # XXX I think this warning is wrong. With multiple inheritance you NEED to > + # take the address after the cast. Ohh right. It is. I was confused by something else. ::: tools/check-moz-style/modules/scm.py @@ +417,5 @@ > + > + def create_patch(self): > + if self.run_command(['hg', 'diff']) != "": > + sys.stderr.write("Warning: outstanding changes not include in style check.\n") > + return self.run_command(['hg', 'export', 'tip']) For now I guess

Comment on attachment 754888 [details] [diff] [review] Part 3: Check patch summary/description + Tests Review of attachment 754888 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/check-moz-style/checkmozstyle.py @@ +111,5 @@ > + cpplint.error("patch", 0, "path/bugnumber", 3, "Patch summary should begin with 'Bug XXXXX - '.") > + > + > + if not patch.patch_description: > + cpplint.error("patch", 0, "path/nodescription", 3, "Patch does not have a description.") Maybe we can just prefix this with Optional: I think most of these warnings should be treated as optional so I don't see a problem with reporting these things as they are useful in knowing. It shouldn't be used to auto r-. ::: tools/check-moz-style/modules/cpplint.py @@ +3099,5 @@ > global _DEFAULT_FILTERS > _DEFAULT_FILTERS = [ > '-whitespace/comments-doublespace', > '-whitespace/blank_line', > + '-build/include', # Webkit specific Alright but lets leave it as a follow up. @@ +3108,5 @@ > '-build/storage_class', # const static > '-build/endif_comment', > '-whitespace/labels', > '-runtime/arrays', # variable length array > '-build/header_guard', agreed

Comment on attachment 774124 [details] [diff] [review] Part 3: Check patch summary/description + Tests + fixes Review of attachment 774124 [details] [diff] [review]: ----------------------------------------------------------------- Let's leave anything else to followups. (I don't really see the distinction between parts 2 and 3; feel free to merge those.) ::: tools/check-moz-style/checkmozstyle.py @@ +89,5 @@ > """ % {'program_name': sys.argv[0]} > > > +def match(pattern, string): > + return re.match(pattern, string) Please just inline this function

Do you have time to get this landed or should I?

(In reply to :Ms2ger from comment #35) > Do you have time to get this landed or should I? Working on some b2g blockers. I can look at this again once its fixed but feel free to land this now if you'd like.

BenWa, Ms2ger: any updates on landing check-moz-style? After check-moz-style lands, it could be integrated with `mach lint` (bug 939350).

No updates; I think about it sometimes, but never end up doing somethng.

FWIW I've been thinking about this from time to time. I think it would be better if someone created a general patch linter that detected the language (based on the patched filename) and had a list of linter appropriate for different extension. This would be more flexible then a specific c++ linter.

Bug 875605 - Import check-webkit-style. r=ms2ger

Bug 875605 - Add HG + update style rules for moz-check-style. r=ms2ger

Bug 875605 - Add tests to check-moz-style. r=ms2ger

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2fedce9b8a8d271698198cab8028e5132bd0bb8 Bug 875605 - Import check-webkit-style. r=ms2ger https://hg.mozilla.org/integration/mozilla-inbound/rev/127f29415cfaa05f10ca2949f54f9bd0b01654c1 Bug 875605 - Add HG + update style rules for moz-check-style. r=ms2ger https://hg.mozilla.org/integration/mozilla-inbound/rev/2c0d1469d2bffa2cd75d6a4696957335d7e6b648 Bug 875605 - Add tests to check-moz-style. r=ms2ger

https://hg.mozilla.org/mozilla-central/rev/d2fedce9b8a8 https://hg.mozilla.org/mozilla-central/rev/127f29415cfa https://hg.mozilla.org/mozilla-central/rev/2c0d1469d2bf