[Webkit-unassigned] [Bug 53672] Modify make_names.pl to not include conditional includes unconditionally
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 15 06:20:09 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=53672
Adam Bergkvist <adam.bergkvist at ericsson.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #81051|0 |1
is obsolete| |
Attachment #89773| |review?
Flag| |
--- Comment #8 from Adam Bergkvist <adam.bergkvist at ericsson.com> 2011-04-15 06:20:09 PST ---
Created an attachment (id=89773)
--> (https://bugs.webkit.org/attachment.cgi?id=89773&action=review)
Updated patch
(In reply to comment #6)
> (From update of attachment 81051 [details])
> 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>.
>
Fixed.
> > 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;
> }
Fixed. (Also fixed the similar if statement in printJSElementIncludes.)
> > 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()?
>
I think printConditionalElementIncludes is OK since the name hints that it only deals with conditional elements (and not feature defined includes in general).
> > Source/WebCore/dom/make_names.pl:624
> > + my $F = shift;
> > + my $wrapperFactoryType = shift;
>
> I would write this as:
>
> my ($F, $wrapperFactoryType) = @_;
>
Fixed.
> > 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.
>
Fixed. (The intention was to make the if statement at line 656 easier to understand.)
> > 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;
>
Fixed.
> > 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.
>
Fixed.
> > 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 .
>
Fixed.
> > 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.
>
I dont't think the curly brackets are optional in this case since I get an error when I remove them.
> > Source/WebCore/dom/make_names.pl:654
> > + print F "#include \"${interfaceName}.h\"\n";
>
> Curly brackets are also used here and are unnecessary.
>
Fixed.
> > Source/WebCore/dom/make_names.pl:656
> > + if ($printJSElementIncludes) {
>
> This can be written as:
>
> if ($wrapperFactoryType) {
>
Fixed.
> > 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.
>
See comment above.
> > 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.
Fixed.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list