Issue 9192010: Parallelize cpplint in presubmit and fix usage of DISALLOW_* macros.

Issue 9192010: Parallelize cpplint in presubmit and fix usage of DISALLOW_* macros. (Closed)

Created:
8 years, 11 months ago by Yang

Modified:
8 years, 11 months ago

Reviewers:
Jakob Kummerow

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Parallelize cpplint in presubmit and fix usage of DISALLOW_* macros. BUG=v8:1653 Committed: https://code.google.com/p/v8/source/detail?r=10465

Patch Set 1 #

Total comments: 8

Created: 8 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+66 lines, -28 lines)			Patch
M	src/full-codegen.h	View	2 chunks	+4 lines, -2 lines	0 comments	Download
M	src/heap.h	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/isolate.h	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/objects.h	View	1 chunk	+2 lines, -1 line	0 comments	Download
M	src/platform-cygwin.cc	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	src/platform-freebsd.cc	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	src/platform-linux.cc	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/platform-macos.cc	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	src/platform-openbsd.cc	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	src/platform-solaris.cc	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	src/platform-win32.cc	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	src/serialize.h	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	tools/presubmit.py	View	4 chunks	+43 lines, -18 lines	8 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Jakob Kummerow

Yay! This is great! LGTM with comments. http://codereview.chromium.org/9192010/diff/1/tools/presubmit.py File tools/presubmit.py (left): http://codereview.chromium.org/9192010/diff/1/tools/presubmit.py#oldcode231 tools/presubmit.py:231: return process.returncode ...

8 years, 11 months ago (2012-01-20 14:56:22 UTC) #2

Yang

8 years, 11 months ago (2012-01-20 16:18:30 UTC) #3

http://codereview.chromium.org/9192010/diff/1/tools/presubmit.py
File tools/presubmit.py (left):

http://codereview.chromium.org/9192010/diff/1/tools/presubmit.py#oldcode231
tools/presubmit.py:231: return process.returncode == 0
On 2012/01/20 14:56:22, Jakob wrote:
> I think we still want a return value here. IIUC, it should be:
> return sum(results) == 0

Done.

http://codereview.chromium.org/9192010/diff/1/tools/presubmit.py
File tools/presubmit.py (right):

http://codereview.chromium.org/9192010/diff/1/tools/presubmit.py#newcode125
tools/presubmit.py:125: child.kill()
On 2012/01/20 14:56:22, Jakob wrote:
> I think you meant s/child/process/

This is actually interesting. Previously the script only reacted to ctrl+c
because child.kill() results in a syntax error and get the worker killed.

But of course your solution is the better one.

http://codereview.chromium.org/9192010/diff/1/tools/presubmit.py#newcode129
tools/presubmit.py:129: child.kill()
On 2012/01/20 14:56:22, Jakob wrote:
> same here.

Done.

http://codereview.chromium.org/9192010/diff/1/tools/presubmit.py#newcode246
tools/presubmit.py:246: results = pool.map(CppLintWorker, commands)
On 2012/01/20 14:56:22, Jakob wrote:
> I need the following patch to make this react properly to Ctrl+C:
> -      results = pool.map(CppLintWorker, commands)
> +      # workaround for http://bugs.python.org/issue8296
> +      results = pool.map_async(CppLintWorker, commands).get(999999)

Done.

Expand Messages | Collapse Messages