195727 - Compiler warning in netwerk/streamconv/converters/mozTXTToHTMLConv.cpp

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[tenthumbs]

Gcc emits this warning.

  mozTXTToHTMLConv.cpp:57: warning: comparison is always false \
      due to limited range of data type

The code is 

  // Bug 183111, editor now replaces multiple spaces with leading
  // 0xA0's and a single ending space, so need to treat 0xA0's as spaces.
  static inline PRBool IsSpace(const char aChar)
  {
    return (nsCRT::IsAsciiSpace(aChar) || aChar == 0xA0);
  }

Gcc knows that -128 <= aChar < 127 so it knows that aChar cannot equal 160
so it eliminates the test.

Aren't clever compilers wonderful?

This was introduced in the fix for bug 183111.

Comment 1

[tenthumbs]

Gcc is happy with something like this:

  static inline PRBool IsSpace(const char aChar)
  {
    unsigned char uc = aChar
    return (nsCRT::IsAsciiSpace(aChar) || uc == 0xA0);
  }

but that's no guarantee any other compiler will like it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

generally, regressions are assigned to people who caused them....

raising severity, since the code will in fact not function correctly due to the
optimization being done by the compiler.

Comment 3

[tenthumbs]

I don't think it's appropriate for me to assign bugs....

Technically it's not an optimization because it's not controlled by the
optimizer (-O?). Note that this only occurs if the default char type is
signed.

Comment 4

[Darin Fisher]

0xA0 == space?  what charset is this anyways?  when i originally reviewed this
patch i just read that as 0x0A ;-)

Comment 5

[timeless]

195 mozTXTToHTMLConv::FindURLStart(const PRUnichar * aInString, PRInt32 aInLength,
253              && !IsSpace(aInString[PRUint32(i)])

ok, let's think about this for a moment. aInString is a pointer to PRUnichar's
which means each is an unsigned 16 bit critter.

and we're going to do what with it?

  static inline PRBool IsSpace(const char aChar)

ah right, pass it to some function which takes a signed 8 bit critter (at least
for some compilers).

The problem here is that IsSpace has a bad prototype, it should *not* take a
signed char.

For reference:
mozilla/editor/libeditor/text/nsInternetCiter.cpp (p5 of 9)
 209 static inline PRBool IsSpace(PRUnichar c)
 211   return (nsCRT::IsAsciiSpace(c) || (c == nl) || (c == cr) || (c == nbsp));

mozilla/string/obsolete/nsString2.cpp (p24 of 27)
 1237 PRBool nsString::IsSpace(PRUnichar aChar) {
 1239   if ((aChar == ' ') || (aChar == '\r') || (aChar == '\n') || (aChar ==
'\t')) {

mozilla/gfx/src/x11shared/nsFT2FontCatalog.cpp (p24 of 45)
 1267 nsFT2FontCatalog::IsSpace(FT_Long c)
 1269   switch (c) {
 1270     case 0x0020: // ascii space
 1271     case 0x00A0: // non-breaking space
 1272     case 0x3000: // ideographic space

Now we have a few options, we could unify the code to use a common function
(nsFT2FontCatalog appears to be the only intl aware impl, so i'd suggest
promoting it), or we could as a poor fix change the prototype to at least take a
PRUnichar like the others.

Comment 6

[Cavin Song]

Taking since I'm the one who caused it. Can we just change IsSpace() to
IsSpace(const PRUnichar aChar)?

Comment 7

[timeless]

Attachment: draft

Comment 8

[timeless]

Comment on attachment 116125 [details] [diff] [review]
draft

ok, well if you want the bug then please review this patch.

Comment 9

[tenthumbs]

>  -    PRInt32 i = pos - 1;
>  +    PRUint32 i = pos - 1;
>       for (; i >= 0 && (         <==============

Since i is now unsigned, i>=0 is always true and gcc eliminates it. Is 
this what you want? You could try i<pos but that's obscure.

Comment 10

[tenthumbs]

     224   case freetext:
     ...
     243   case abbreviated:

Both cases have "PRInt32 i = pos -1". Shouldn't they be treated the same 
in any patch.

Comment 11

[Ben Bucksch (:BenB)]

timeless, be *very* careful before changing these signed to unsigned. I think
you just introduced a bug. That pos - 1 and then the check for >= was there
*intentionally*, IIRC. pos can be 0 and we need to check the char *before* pos.
But it could have been the beginning, in which case we can't and a certain case
triggers.

Comment 12

[Cavin Song]

Comment on attachment 116125 [details] [diff] [review]
draft

Any reason why we need to change/rewrite the logic of:

if (++i >= 0 && i < pos && nsCRT::IsAsciiAlpha(aInString[PRUint32(i)]))?

Not sure if the new one is right? For example, assuming i=3 and pos=4, the
existing code would return PR_TRUE but the new code retruns PR_FALSE.

Comment 13

[Ben Bucksch (:BenB)]

This
-static inline PRBool IsSpace(const char aChar)
+static inline PRBool IsSpace(PRUnichar aChar)
should be enough to fix this bug, not?

Comment 14

[tenthumbs]

> This
> -static inline PRBool IsSpace(const char aChar)
> +static inline PRBool IsSpace(PRUnichar aChar)
> should be enough to fix this bug, not?

Yes, the warning's gone and no other one appears. But that's Linux gcc-3.2.2.
Obviously needs to be tested on other platforms.

Comment 15

[Cavin Song]

Attachment: Proposed patch, v1

timeless, feel free to take it back from me. either way works for me.

Comment 16

[Darin Fisher]

Comment on attachment 116217 [details] [diff] [review]
Proposed patch, v1

r=darin 

please comment that 0xA0 is the Latin1/Unicode character for "non-breaking
space (nbsp)" ... thx!

Comment 17

[tenthumbs]

I hate to be rude but has someone actually checked that the patch works 
without compiler warnings on non-Linux platforms? This bug exists only 
because no one looked at the gcc output.

Comment 18

[(not reading, please use [e-mail] instead)]

Comment on attachment 116217 [details] [diff] [review]
Proposed patch, v1

sr=sspitzer

Comment 19

[Cavin Song]

Fix checked in.

Comment 20

[(not reading, please use [e-mail] instead)]

as dmose is about to point out, this should have been:

-static inline PRBool IsSpace(const char aChar)
+static inline PRBool IsSpace(const PRUnichar aChar)

not

-static inline PRBool IsSpace(const char aChar)
+static inline PRBool IsSpace(PRUnichar aChar)

he'll check in that change soon.  dmose, can you comment once you do?

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

const qualifier has been checked in.