Bug 74771 – Remove the duplicated code from ASCIICType.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Benjamin Poulain]

ASCIICType.h is just a bunch of function copied for every type. I have heard of that template thingy that is good at doing that ;)

[Benjamin Poulain]

Created attachment 119703 [details]
Patch

[Benjamin Poulain]

I was also curious why we need to support so many types and not just UChar for every function... It is because of the JSC::Yarr() and JSC::lexer(), which use various types for the characters.

This could probably be fixed but I preferred to limit the scope of this particular change.

[Andreas Kling]

Comment on attachment 119703 [details]
Patch

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

Great idea!

> Source/JavaScriptCore/ChangeLog:8
> +        The functions were sharing similar code and were defined for the various input type.

type -> types

> Source/JavaScriptCore/ChangeLog:9
> +        Use template instead to avoid code duplication.

template -> templates

> Source/JavaScriptCore/wtf/ASCIICType.h:98
> + character          count
> + ---------          -----
> + non-spaces         689383
> + 20  space              294720
> + 0A  \n                 89059
> + 09  \t                 28320
> + 0D  \r                 0
> + 0C  \f                 0
> + 0B  \v                 0
> + */

Looks like you're breaking the alignment here a bit.

[Benjamin Poulain]

Committed r103144: <http://trac.webkit.org/changeset/103144>

[Benjamin Poulain]

Reopen, I am not finished here :)

[Benjamin Poulain]

Created attachment 119715 [details]
Patch

[Andreas Kling]

Comment on attachment 119715 [details]
Patch

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

> Source/JavaScriptCore/wtf/ASCIICType.h:58
> +    return (c >= '0') && (c <= '9');

You don't need parentheses here. NABD.

[Benjamin Poulain]

Committed r103202: <http://trac.webkit.org/changeset/103202>

[Benjamin Poulain]

(In reply to comment #7)
> > Source/JavaScriptCore/wtf/ASCIICType.h:58
> > +    return (c >= '0') && (c <= '9');
> 
> You don't need parentheses here. NABD.

I know, but I like the parentheses :)

The helmet is not mandatory when rollerblading(*), but it is safer ;)


(*) restrictions may apply depending on your country. The lack of helmet can have side effects including nausea, headache or death. If you feel any discomfort, immediately consult a doctor.

[Alexey Proskuryakov]

The first patch is marked as obsolete. Was it rolled out? If it's in the tree, it should not be obsolete, as otherwise it's too hard to see what changes the bug included.

[Benjamin Poulain]

(In reply to comment #10)
> The first patch is marked as obsolete. Was it rolled out? If it's in the tree, it should not be obsolete, as otherwise it's too hard to see what changes the bug included.

You are right. I used webkit-patch without thinking about that.