[webkit-reviews] review denied: [Bug 206389] It should be possible to build JavaScriptCore with LLVM Source-based Code Coverage, run the tests and see the coverage data : [Attachment 388427] proposed patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied Tuomas Karkkainen
<tuomas.webkit at apple.com>'s request for review:
Bug 206389: It should be possible to build JavaScriptCore with LLVM
Source-based Code Coverage, run the tests and see the coverage data
https://bugs.webkit.org/show_bug.cgi?id=206389

Attachment 388427: proposed patch

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




--- Comment #16 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 388427
  --> https://bugs.webkit.org/attachment.cgi?id=388427
proposed patch

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

r- to fix if/else formatting.

> Tools/ChangeLog:10
> +	   * Scripts/run-javascriptcore-tests:
> +	   * Scripts/set-webkit-configuration:
> +	   * Scripts/webkitdirs.pm:

Can we make `Tools/Scripts/set-webkit-configuration --coverage` take an
optional string argument with the path to the coverage directory and write the
string path to the Coverage text file instead of "YES"?

If we did that, then run-javascriptcore-tests wouldn't require any argument to
run with coverage (it could just read the directory from the Coverage config
file).

This would also simplify the command-line switches for all the scripts to just
a single "--coverage[=s]" switch that takes an optional path argument.

> Tools/Scripts/run-javascriptcore-tests:221
> +my $coverageDefault = $coverage ? "coverage enabled" : "coverage disabled";

Should run-javascfriptcore-tests set its default value based on reading the
Coverage config file (determineCoverageIsEnabled() from webkitdirs.pm) if there
is no command-line switch?  Seems like we'd want this to "just work".

> Tools/Scripts/run-javascriptcore-tests:513
> +if ($coverage)
> +{
> +    if ($coverageDir)
> +    {
> +	   print "Using output path specified on the command line for coverage
data: $coverageDir\n";
> +    }
> +    else
> +    {
> +	   $coverageDir = tempdir();
> +	   print "Generating coverage data into $coverageDir\n";
> +    }

Please correct formatting of braces in if/else blocks (I'm not going to call
out every instance of this, so here's an example):

if ($coverage) {
    if ($coverageDir) {
	print "Using output path specified on the command line for coverage
data: $coverageDir\n";
    } else {
	$coverageDir = tempdir();
	print "Generating coverage data into $coverageDir\n";
    }

> Tools/coverage/coverage.xcconfig:3
> +OTHER_CFLAGS = $(inherited) -fprofile-instr-generate -fcoverage-mapping;
> +OTHER_CPLUSPLUSFLAGS = $(inherited) -fprofile-instr-generate
-fcoverage-mapping;
> +OTHER_LDFLAGS = $(inherited) -fprofile-instr-generate;

To answer Alexey's question from another patch review:	Yes, $(inherited)
should do the correct thing with asan.xcconfig files.