[webkit-reviews] review granted: [Bug 205787] Add --forceOpt option to Tools/Scripts/set-webkit-configuration. : [Attachment 386866] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 205787: Add --forceOpt option to Tools/Scripts/set-webkit-configuration.
https://bugs.webkit.org/show_bug.cgi?id=205787

Attachment 386866: proposed patch.

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




--- Comment #7 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 386866
  --> https://bugs.webkit.org/attachment.cgi?id=386866
proposed patch.

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

> Tools/Scripts/set-webkit-configuration:41
> +  --forceOptimizationLevel=<level>	Optimization level: O3, O2, O1, O0, Os,
Ofast, Og, or none

why is this camel case? shouldn't this be "--force-optimization-level" to be
consistent with other options?

> Tools/Scripts/webkitdirs.pm:429
> +    return if defined $forceOptimizationLevel;

would this ever actually be defined in memory? What if it were "none" here?
Shouldn't we just always do below?

> Tools/Scripts/webkitdirs.pm:435
> +	   chomp $forceOptimizationLevel;

small nit: why write the newline to the file and then do this? Why not just
omit "\n" from the file?