[webkit-reviews] review granted: [Bug 27323] Better support for non-Cygwin SVN on Windows : [Attachment 33219] Fix WebCore/DerivedSources.make "Duplicate value!" problem

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Peter Kasting
<pkasting at google.com>'s request for review:
Bug 27323: Better support for non-Cygwin SVN on Windows
https://bugs.webkit.org/show_bug.cgi?id=27323

Attachment 33219: Fix WebCore/DerivedSources.make "Duplicate value!" problem
https://bugs.webkit.org/attachment.cgi?id=33219&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> -	if sort $(WEBCORE_CSS_PROPERTY_NAMES) | uniq -d | grep -E '^[^#]'; then
echo 'Duplicate value!'; exit 1; fi
> +	if sort $(WEBCORE_CSS_PROPERTY_NAMES) | uniq -d | grep -Ev
'(^#)|(^[[:space:]]*$)'; then echo 'Duplicate value!'; exit 1; fi

Nit: I generally like to put spaces between arguments (rather than merging
them):	"-E -v".  This isn't necessary, though.

> -	if sort CSSValueKeywords.in | uniq -d | grep -E '^[^#]'; then echo
'Duplicate value!'; exit 1; fi
> +	if sort CSSValueKeywords.in | uniq -d | grep -Ev
'(^#)|(^[[:space:]]*$)'; then echo 'Duplicate value!'; exit 1; fi
>	perl "$(WebCore)/css/makevalues.pl"

Change this to "-E -v", too, if you change the above command.

> Index: WebCore/css/makeprop.pl
> ===================================================================
> --- WebCore/css/makeprop.pl	(revision 46178)
> +++ WebCore/css/makeprop.pl	(working copy)
> @@ -26,9 +26,9 @@ use warnings;
>  open NAMES, "<CSSPropertyNames.in" || die "Could not find
CSSPropertyNames.in";
>  my @names = ();
>  while (<NAMES>) {
> -  next if (m/#/);
> -  chomp $_;
> -  next if ($_ eq "");
> +  next if (m/(^#)|(^\s*$)/);
> +  # Input may use a different EOL sequence than $/, so avoid chomp.
> +  $_ =~ s/[\r\n]+$//g;
>    push @names, $_;
>  }
>  close(NAMES);

I think it may be safest to put the \r\n replacement before the "next" line
unless you're sure that Perl is going to treat all forms of line endings
equally on all platforms when applying regular expressions.  (I don't know the
answer myself, but it's more common to chomp() before you try to match, so I'd
follow that pattern anyway.)

> Index: WebCore/css/makevalues.pl
> ===================================================================
> --- WebCore/css/makevalues.pl (revision 46178)
> +++ WebCore/css/makevalues.pl (working copy)
> @@ -26,9 +26,9 @@ use warnings;
>  open NAMES, "<CSSValueKeywords.in" || die "Could not open
CSSValueKeywords.in";
>  my @names = ();
>  while (<NAMES>) {
> -  next if (m/#/);
> -  chomp $_;
> -  next if ($_ eq "");
> +  next if (m/(^#)|(^\s*$)/);
> +  # Input may use a different EOL sequence than $/, so avoid chomp.
> +  $_ =~ s/[\r\n]+$//g;
>    push @names, $_;
>  }
>  close(NAMES);

Ditto.

r=me with the changes to makeprop.pl and makevalues.pl.