LGTM if comments are addressed.

Thanks for contributing!


http://codereview.chromium.org/10335014/diff/1/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/10335014/diff/1/build/common.gypi#newcode281
build/common.gypi:281: ['v8_target_arch!="x64"', {
The condition must check for OS as well, as the shell commands are not
available e.g. on Windows.
Also, I have a weak preference to explicitly list the affected
architectures, but I don't feel strongly about that.
The following should work:

['(OS=="linux" or OS=="freebsd" or OS=="openbsd" or \
   OS=="solaris" or OS=="netbsd" or OS=="mac") and \
  (v8_target_arch=="arm" or v8_target_arch=="ia32" or \
   v8_target_arch=="mips")', {

(Mac is tricky since you can use GYP to generate either Xcode project
files or Makefiles there. In order not to break the latter, I think we
have to include OS=="mac" here, but I might be wrong. Do you have a Mac
around where you can test this?)

http://codereview.chromium.org/10335014/diff/1/build/common.gypi#newcode282
build/common.gypi:282: # Check whether the host compiler and target
compiler support '-m32' option and set it if yes
nit: for comments, please keep the 80-col limit per line, and end
sentences with a full stop.

http://codereview.chromium.org/10335014/

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to