[webkit-reviews] review granted: [Bug 53672] Modify make_names.pl to not include conditional includes unconditionally : [Attachment 81051] Proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 11 20:29:06 PDT 2011
Daniel Bates <dbates at webkit.org> has granted Adam Bergkvist
<adam.bergkvist at ericsson.com>'s request for review:
Bug 53672: Modify make_names.pl to not include conditional includes
unconditionally
https://bugs.webkit.org/show_bug.cgi?id=53672
Attachment 81051: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=81051&action=review
------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81051&action=review
> Source/WebCore/ChangeLog:10
> + No new tests (no change in functionality)
I wish we could test this :-(. I don't think we have unit test coverage for
make_names.pl (since it's a driver program as opposed to a Perl module).
> Source/WebCore/ChangeLog:12
> + * dom/make_names.pl:
Because prepareChangeLog doesn't support Perl, it is convention that we add
analogous notes that document the added functions. This makes it convenient for
a person to determine which changeset added/changed a particular function by
searching the change log entries for the function name. For an example of this,
see <http://trac.webkit.org/changeset/52692>.
> Source/WebCore/dom/make_names.pl:614
> + next if $enabledTags{$tagName}{conditional} ||
defined($tagsSeen{$interfaceName});
I suggest adding a comment above this line that explains that we are skipping
feature-define-specific #includes because we will handle such #includes
separately. OR, even better, break the disjuncts into separate if statements,
like:
next if defined($tagsSeen{$interfaceName});
if ($enabledTags{$tagName}{conditional}) {
# We skip feature-define-specific #includes here since will we handle such
#includes separately.
next;
}
> Source/WebCore/dom/make_names.pl:621
> +sub printConditionalElementIncludes
This function only handles feature-define #includes as opposed to all such
conditional #include. Maybe a better name would be
printFeatureDefineIncludes()?
> Source/WebCore/dom/make_names.pl:624
> + my $F = shift;
> + my $wrapperFactoryType = shift;
I would write this as:
my ($F, $wrapperFactoryType) = @_;
> Source/WebCore/dom/make_names.pl:625
> + my $printJSElementIncludes = !!$wrapperFactoryType;
It is necessary to explicitly convert $wrapperFactoryType to a boolean value
(see my comment for line 656). Moreover, this variable is unnecessary since
it's being referenced only once in this function; => the value should just be
inlined into the if-statement.
> Source/WebCore/dom/make_names.pl:629
> + my %conditionals = ();
> + my %unconditionalElementIncludes = ();
> + my %unconditionalJSElementIncludes = ();
It is unnecessary to explicitly initialize these. You can just write these as:
my %conditionals;
my %unconditionalElementIncludes;
my %unconditionalJSElementIncludes;
> Source/WebCore/dom/make_names.pl:641
> + if (!$conditionals{$conditional}) {
> + $conditionals{$conditional} = ();
> + $conditionals{$conditional}{interfaceNames} = ();
> + $conditionals{$conditional}{JSInterfaceNames} = ();
> + }
This code is unnecessary. No need to explicitly initialize the values of these
hash keys. Perl is reasonable when it comes it using undefined values.
> Source/WebCore/dom/make_names.pl:651
> + print F "\n#if ENABLE(${conditional})\n";
The curly bracket '{' and '}' are unnecessary. It seems that make_names.pl
doesn't have a consistent notation. Some code uses the curly-bracket notation
and some doesn't. Regardless, I suggest removing the curly brackets since there
is only one variable to be interpolated and I don't feel there presence
improves the readability of this string, which already feels crowded by the
presence of all the surrounding punctuation characters (e.g. '(', ')') and
new-line escape sequences .
> Source/WebCore/dom/make_names.pl:652
> + for my $interfaceName (sort keys
%{$conditionals{$conditional}{interfaceNames}}) {
I would remove the curly brackets around the expression $conditionals{...}{...}
as they are unnecessary.
> Source/WebCore/dom/make_names.pl:654
> + print F "#include \"${interfaceName}.h\"\n";
Curly brackets are also used here and are unnecessary.
> Source/WebCore/dom/make_names.pl:656
> + if ($printJSElementIncludes) {
This can be written as:
if ($wrapperFactoryType) {
> Source/WebCore/dom/make_names.pl:657
> + for my $JSInterfaceName (sort keys
%{$conditionals{$conditional}{JSInterfaceNames}}) {
I would remove the curly brackets around the expression $conditionals{...}{...}
as they are unnecessary.
> Source/WebCore/dom/make_names.pl:713
> +printConditionalElementIncludes($F, "");
Leave off the second argument; it's syntactically unnecessary and doesn't add
any value.
Notice, when you leave off the second argument then $wrapperFactoryType is
undefined inside the function body printConditionalElementIncludes(); =>
$wrapperFactoryType evaluates to false in the if-statment on line 656.
More information about the webkit-reviews
mailing list