Fortunately, I have a working internet connection and a few spare minutes
every
now and then :-)
There's no need to update SCons-related files, as V8's SCons build is dead
and
about to be removed anyway.
https://codereview.chromium.org/11574031/diff/1/Makefile
File Makefile (right):
https://codereview.chromium.org/11574031/diff/1/Makefile#newcode81
Makefile:81: ifeq ($(vtunejit), on)
nit: indentation
https://codereview.chromium.org/11574031/diff/1/Makefile#newcode82
Makefile:82: GYPFLAGS += -Dv8_enable_vtunejit=1
nit: indentation
https://codereview.chromium.org/11574031/diff/1/build/common.gypi
File build/common.gypi (right):
https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode77
build/common.gypi:77: 'v8_enable_vtunejit%': 0,
A short comment would be nice, e.g.:
# Enable support for Intel VTune. Supported on ia32/x64 only.
https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode94
build/common.gypi:94: #'werror%': '',
what's this?
https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode130
build/common.gypi:130: 'conditions': [
nit: indentation
https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode131
build/common.gypi:131: ['v8_target_arch=="ia32" or
v8_target_arch=="x64"', {
A 'conditions' section inside a 'conditions' section? You can just
combine the conditions:
['v8_enable_vtunejit==1 and \
(v8_target_arch=="ia32" or v8_target_arch=="x64")', {
'defines': ...
On the other hand, the define and the libary don't hurt for other target
archs, right? Since v8_enable_vtunejit defaults to 0, you could just
skip the v8_target_arch condition, and leave it up to the user to
realize that VTune doesn't support non-Intel architectures.
https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode134
build/common.gypi:134: #dw -ljitprofiling',],
what's this?
https://codereview.chromium.org/11574031/diff/1/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):
https://codereview.chromium.org/11574031/diff/1/tools/gyp/v8.gyp#newcode757
tools/gyp/v8.gyp:757: ['v8_enable_vtunejit==1', {
Inclusion of source files should occur on the same condition as the
define/link settings (see my comments there). If you want to keep the
dependency on target architecture, you should introduce an internal
variable so you can avoid the duplication of the condition.
https://codereview.chromium.org/11574031/diff/1/tools/gyp/v8.gyp#newcode758
tools/gyp/v8.gyp:758: 'sources' : [
nit: no space before ':'
https://codereview.chromium.org/11574031/diff/1/tools/gyp/v8.gyp#newcode759
tools/gyp/v8.gyp:759: '../../src/third_party/vtune/vtune-jit.cc',
nit: indentation
https://codereview.chromium.org/11574031/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
