[webkit-reviews] review denied: [Bug 175216] Separate jsc stress test script writer from run-jsc-stress-tests : [Attachment 317504] Separate out script building/running from other parts of run-jsc-stress-tests

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Stephan Szabo
<stephan.szabo at sony.com>'s request for review:
Bug 175216: Separate jsc stress test script writer from run-jsc-stress-tests
https://bugs.webkit.org/show_bug.cgi?id=175216

Attachment 317504: Separate out script building/running from other parts of
run-jsc-stress-tests

https://bugs.webkit.org/attachment.cgi?id=317504&action=review




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 317504
  --> https://bugs.webkit.org/attachment.cgi?id=317504
Separate out script building/running from other parts of run-jsc-stress-tests

View in context: https://bugs.webkit.org/attachment.cgi?id=317504&action=review

r- because parseRunCommands is duplicated.

> Tools/Scripts/webkitruby/jsc-stress-test-writer-default.rb:217
> +#$runCommandOptions = {}

Why have this here?n  This looks like a copy-paste that hasn't been cleaned up.

> Tools/Scripts/webkitruby/jsc-stress-test-writer-default.rb:320
> +# Returns true if there were run commands found in the file
($benchmarkDirectory +
> +# $benchmark), in which case those run commands have already been executed.
Otherwise
> +# returns false, in which case you're supposed to add your own run commands.
> +def parseRunCommands
> +    oldDidAddRunCommand = $didAddRunCommand
> +    $didAddRunCommand = false
> +
> +    Dir.chdir($outputDir) {
> +	   File.open($benchmarkDirectory + $benchmark) {
> +	       | inp |
> +	       inp.each_line {
> +		   | line |
> +		   begin
> +		       doesMatch = line =~ /^\/\/@/
> +		   rescue Exception => e
> +		       # Apparently this happens in the case of some UTF8 stuff
in some files, where
> +		       # Ruby tries to be strict and throw exceptions.
> +		       next
> +		   end
> +		   next unless doesMatch
> +		   eval $~.post_match
> +	       }
> +	   }
> +    }
> +
> +    result = $didAddRunCommand
> +    $didAddRunCommand = result or oldDidAddRunCommand
> +    result
> +end

There's still a copy of this function in run-jsc-stress-tests.	Is there a
reason for this?