[webkit-reviews] review denied: [Bug 72128] CMake color output is missing when using build-webkit : [Attachment 114877] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Ming Xie <mxie at rim.com>'s request
for review:
Bug 72128: CMake color output is missing when using build-webkit
https://bugs.webkit.org/show_bug.cgi?id=72128

Attachment 114877: Patch
https://bugs.webkit.org/attachment.cgi?id=114877&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114877&action=review


This patch is good. I have some remarks on how we can improve this patch.

> Tools/Scripts/filter-build-webkit:46
> +    CMAKE_STYLE_RED => 4,
> +    CMAKE_STYLE_GREEN => 5,
> +    CMAKE_STYLE_BLUE => 6,
> +    CMAKE_STYLE_CYAN=> 7,

Can we give these variables names that better reflect their purpose? For
instance, we use style CMAKE_STYLE_GREEN whenever we are building/processing a
source file. Maybe we should rename CMAKE_STYLE_GREEN to
CMAKE_STYLE_BUILDING_SOURCE?

> Tools/Scripts/filter-build-webkit:75
>  sub setLogfileOption($$);
>  sub setOutputFormatOption($$);
>  sub usageAndExit();
> +sub processOutputWithCMakeColor($);

These forward declarations should be in sorted order.

>> Tools/Scripts/filter-build-webkit:103

> 
> I think merging with --[no-]color makes sence.

As far as I can tell the regular expressions for Xcodebuild don't overlap with
the regular expressions for CMake. So, we could merge the CMake coloring mode
into the command-line option color.

> Tools/Scripts/filter-build-webkit:229
> +    if ($line =~ /.*(Linking CXX|error:).*/) {

Does this regular expression match when linking non-C++ files (e.g. C files)?
Would it be sufficient to match for "Linking"?

It's unnecessary to have ".*" at the beginning and end of the regular
expression as Perl regular expressions match a substring. Also, the parentheses
in the regular expression aren't necessary and don't seem to improve the
readability of this expression. So, I would write the regular expression in
this line as: /Linking CXX|error:/.

See <http://perldoc.perl.org/perlrequick.html#The-Guide> and
<http://perldoc.perl.org/perlre.html> for more details.

> Tools/Scripts/filter-build-webkit:230
> +	   printLine("$line", CMAKE_STYLE_RED);

It's sufficient to pass $line directly as the first argument of printLine()
instead of using string interpolation:

printLine($line, CMAKE_STYLE_RED);

Notice the lack of double quotes around the first argument of printLine().

> Tools/Scripts/filter-build-webkit:231
> +    } elsif ($line =~ /.*(Building|Compiling|Preprocessing) CXX
(object|source).*/) {

Does this regular expression match for non-C++ files (e.g. C files)?

As mentioned in the remark for line 229, it's unnecessary to prefix and append
".*" to the regular expression.

> Tools/Scripts/filter-build-webkit:232
> +	   printLine("$line", CMAKE_STYLE_GREEN);

Remove the quotes around variable $line. See the remark on line 229 for more
details.

> Tools/Scripts/filter-build-webkit:233
> +    } elsif ($line =~ /.*Generating.*/) {

The regular expression on this line can be written:

/Generating/

> Tools/Scripts/filter-build-webkit:234
> +	   printLine("$line", CMAKE_STYLE_BLUE);

Remove the quotes around variable $line. See the remark on line 229 for more
details.

> Tools/Scripts/filter-build-webkit:235
> +    } elsif ($line =~ /.*(Run.*(CPack|CMake)|Install.*project|Available
install components).*/) {

Do we really want to match zero or more arbitrary characters between "Run" and
"CPack", and "Run" and "CMake"? Would it be sufficient to match whitespace
(\s)? Can we make this regular expression stronger?

A similar argument can be made for "Install.*project".

> Tools/Scripts/filter-build-webkit:236
> +	   printLine("$line", CMAKE_STYLE_CYAN);

Remove the quotes around variable $line. See the remark on line 229 for more
details.

> Tools/Scripts/filter-build-webkit:238
> +	   printLine("$line", STYLE_PLAIN);

Remove the quotes around variable $line. See the remark on line 229 for more
details.