Bug 15519 – eliminate use of <ctype.h> for processing ASCII

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

[Darin Adler]

Since <ctype.h> can give locale-specific results, it's not good to use it in WebKit. Also, it's not necessarily fast or efficient enough. We'll use our own instead.

[Darin Adler]

Committed revision 26638.

[Darin Adler]

(In reply to comment #1)
> Committed revision 26638.

Oops. That was for another bug.

[Darin Adler]

Created attachment 16683 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 16683 [details]
patch

I'm a big fan of this change.  Those method names are so much cleaner than the ctype ones, and it's nice to get rid of the ctype include hack.  I was a little surprised you didn't use template functions to implement the various copy/paste char variants.  I would mark this r+ but I didn't stare at the individual is* functions long enough to determine if their actually 100% correct.  I expect they are, but I don't trust my brain at this hour.

[Geoffrey Garen]

-          if (isalpha(*p) && toupper(*p) != *p)
+          if (isASCIIAlpha(*p) && toASCIILower(*p) != *p)
             lstyle = 0;

As an EBay reviewer once wrote, "BAD BAD FRAUD."

[Darin Adler]

(In reply to comment #5)
> -          if (isalpha(*p) && toupper(*p) != *p)
> +          if (isASCIIAlpha(*p) && toASCIILower(*p) != *p)
>              lstyle = 0;
> 
> As an EBay reviewer once wrote, "BAD BAD FRAUD."

Oops! I wonder why this one didn't show up as a test failure. I'll look into that.

[Maciej Stachowiak]

Comment on attachment 16683 [details]
patch

Would it be reasonable to make these template functions instead of overloads:

+inline bool isASCIIAlpha(char c) { return (c | 0x20) >= 'a' && (c | 0x20) <= 'z'; }
+inline bool isASCIIAlpha(unsigned short c) { return (c | 0x20) >= 'a' && (c | 0x20) <= 'z'; }
+inline bool isASCIIAlpha(wchar_t c) { return (c | 0x20) >= 'a' && (c | 0x20) <= 'z'; }

Perhaps DisallowCType.h or ASCIICType.h should have a comment explaining why ASCIICType functions are preferred to regular ctype functions.

r=me anyway but please consider these suggestions.

[Geoffrey Garen]

I verified with a table that the ASCII functions are correct.

I agree with Maciej that a better explanation / compiler error would help a developer who run into it down the line.

5948          } else if (c == ' ' || ispunct(c)) {
 5949         } else if (c == ' ' || c >= 0x21 && c <= 0x2F || c >= 0x3A && c <= 0x40 || c >= 0x5B && c <= 0x60 || c >= 0x7B && c <= 0x7D) {
59495950   

Why not provide isASCIIPunct instead?

For ASCIICType.h, I think the preferred style is to indent inside a namespace in header files. 

Also, why so much duplicated code? For instance, why doesn't isASCIIAlphanumeric call isASCIIAlpha?

r=me

[Geoffrey Garen]

I verified with a table that the ASCII functions are correct.

I agree with Maciej that a better explanation / compiler error would help a developer who run into it down the line.

5948          } else if (c == ' ' || ispunct(c)) {
 5949         } else if (c == ' ' || c >= 0x21 && c <= 0x2F || c >= 0x3A && c <= 0x40 || c >= 0x5B && c <= 0x60 || c >= 0x7B && c <= 0x7D) {
59495950   

Why not provide isASCIIPunct instead?

For ASCIICType.h, I think the preferred style is to indent inside a namespace in header files. 

Also, why so much duplicated code? For instance, why doesn't isASCIIAlphanumeric call isASCIIAlpha?

r=me

[Darin Adler]

(In reply to comment #9)
> -         } else if (c == ' ' || ispunct(c)) {
> +         } else if (c == ' ' || c >= 0x21 && c <= 0x2F || c >= 0x3A && c
> <= 0x40 || c >= 0x5B && c <= 0x60 || c >= 0x7B && c <= 0x7D) {
> 
> Why not provide isASCIIPunct instead?

To me, this category seems far less definitive than the others. And the rule is specifically about Mac OS X behavior. I suspect that ispunct isn't even the correct rule; at some point we could visit this code and get the rule exactly right for all sorts of keyboards. I don't think it's correct to assume that the only punctuation you could type on a keyboard is ASCII punctuation.

I didn't want to dignify the existing rule with a name, but I also didn't want to change behavior.

> For ASCIICType.h, I think the preferred style is to indent inside a namespace
> in header files.

Hyatt and Maciej have been debating this point. I could go either way.

> Also, why so much duplicated code? For instance, why doesn't
> isASCIIAlphanumeric call isASCIIAlpha?

Hard to say exactly why. I like the fact that each of these lines stands alone. I expect that any future changes to this file would primarily be for performance optimization, perhaps to remove some of these functions if we find they aren't really useful, or maybe to add a few more.

[Darin Adler]

(In reply to comment #7)
> (From update of attachment 16683 [details] [edit])
> Would it be reasonable to make these template functions instead of overloads:
> 
> +inline bool isASCIIAlpha(char c) { return (c | 0x20) >= 'a' && (c | 0x20) <=
> 'z'; }
> +inline bool isASCIIAlpha(unsigned short c) { return (c | 0x20) >= 'a' && (c |
> 0x20) <= 'z'; }
> +inline bool isASCIIAlpha(wchar_t c) { return (c | 0x20) >= 'a' && (c | 0x20)
> <= 'z'; }

Maybe. Template functions are more annoying when debugging. I'm feeling slightly conflicted about it, and I think I'll leave it as-is for now.

[Darin Adler]

Committed revision 26676.