Bug 65046 – Add parsing support for extended attributes on IDL constants

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Add parsing support for extended attributes on IDL constants

[Patrick R. Gansterer]

Created attachment 101766 [details]
Patch

[Brent Fulgham]

Comment on attachment 101766 [details]
Patch

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

Looks good. r+, but please consider my suggestions.

> Source/WebCore/bindings/scripts/IDLParser.pm:332
> +                my $constExtendedAttributes = (defined($1) ? $1 : " "); chop($constExtendedAttributes);

I'm not a fan of having two bits of logic on a single line like this.  I'd rather see the 'chop' on its own line.

> Source/WebCore/bindings/scripts/IDLStructure.pm:97
> +our $constantSelector = 'const\s*(' . $extendedAttributeSyntax . ' )?' . $supportedTypes . '\s*(' . $idlType . '*)\s*=\s*(' . $constValue . ')';

Does adding this new elements to the IDL structure require lots of the existing IDL to be changed?  I think the answer is no (based on the EWS), but I just wanted to double-check.

[Patrick R. Gansterer]

Comment on attachment 101766 [details]
Patch

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

>> Source/WebCore/bindings/scripts/IDLParser.pm:332
>> +                my $constExtendedAttributes = (defined($1) ? $1 : " "); chop($constExtendedAttributes);
> 
> I'm not a fan of having two bits of logic on a single line like this.  I'd rather see the 'chop' on its own line.

It's only the same code as in ...
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L216
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L245
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L279
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L312

>> Source/WebCore/bindings/scripts/IDLStructure.pm:97
>> +our $constantSelector = 'const\s*(' . $extendedAttributeSyntax . ' )?' . $supportedTypes . '\s*(' . $idlType . '*)\s*=\s*(' . $constValue . ')';
> 
> Does adding this new elements to the IDL structure require lots of the existing IDL to be changed?  I think the answer is no (based on the EWS), but I just wanted to double-check.

as the ? in the regexp already says: it's optional :-)

[Brent Fulgham]

Comment on attachment 101766 [details]
Patch

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

Re-approved.  r+

>>> Source/WebCore/bindings/scripts/IDLParser.pm:332
>>> +                my $constExtendedAttributes = (defined($1) ? $1 : " "); chop($constExtendedAttributes);
>> 
>> I'm not a fan of having two bits of logic on a single line like this.  I'd rather see the 'chop' on its own line.
> 
> It's only the same code as in ...
> http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L216
> http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L245
> http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L279
> http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/IDLParser.pm?rev=91770#L312

Okay -- existing precedent trumps personal opinion :-)

>>> Source/WebCore/bindings/scripts/IDLStructure.pm:97
>>> +our $constantSelector = 'const\s*(' . $extendedAttributeSyntax . ' )?' . $supportedTypes . '\s*(' . $idlType . '*)\s*=\s*(' . $constValue . ')';
>> 
>> Does adding this new elements to the IDL structure require lots of the existing IDL to be changed?  I think the answer is no (based on the EWS), but I just wanted to double-check.
> 
> as the ? in the regexp already says: it's optional :-)

Great!

[WebKit Review Bot]

Comment on attachment 101766 [details]
Patch

Clearing flags on attachment: 101766

Committed r94010: <http://trac.webkit.org/changeset/94010>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.