456502 - nsFind::Find() calls StripChars() on a non-ASCII char

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Serge Gautherie (:sgautherie)]

Bug 450048 comment 4
{
From  Boris Zbarsky (todo: 200+ items)   2008-08-20 14:40:11 PDT

The code that calls StripChars on a non-ASCII char is wrong
}

I'm not sure what the issue is with non-ASCII chars,
nor how to fix it.

***

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#175
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.cpp#142

There might (not) be a few other cases !?
http://mxr.mozilla.org/mozilla-central/search?string=StripChars&case=on

Comment 1

[Boris Zbarsky [:bzbarsky]]

The call will remove the wrong char.  Instead of removing the char that you want, it'll remove that modulo 255.

The right way to fix it is to use an equivalent PRUnichar api, or to hand-write the StripChars code using PRUnichars.

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> it'll remove that modulo 255.

The point I don't get is ASCII 7 bit vs char 8 bit:
|#define CH_SHY ((PRUnichar) 0xAD)|, especially as a |char|, is < 255, isn't it ?

> The right way to fix it is to use an equivalent PRUnichar api

Is there one ?
Should one be added to the String API ?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, if CH_SHY is < 255, then this code is probably OK, though then it needs a big comment (and better yet a PR_STATIC_ASSERT) about that.

Comment 4

[Saint Wesonga]

Attachment: Patch

Seems to me the <= operator is more appropriate.

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/18ca254da034