1129048 - Don't use 0xFF as EOF marker - Bug 1085151 follow up.

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Andreas Schwab]

Attachment: Use out-of-bounds value as EOF marker.

+++ This bug was initially created as a clone of Bug #1085151 +++

seamonkey fails to build on powerpc with the following type of errors

comm-central/mailnews/base/search/src/nsMsgFilterList.cpp:493:22: error: comparison is always true due to limited range of data type [-Werror=type-limits]
   while (curChar != -1);

'char' on powerpc is unsigned.

Comment 1

[:aceman]

You copied the bug description from bug 1085151 without any change. So why do you propose reverting the patch in that bug? Please explain. I see some discussion in that bug so please copy the relevant parts here. Because the build on powerpc probably isn't failing for you, so the description is bogus.

Comment 2

[Andreas Schwab]

I don't revert the patch.  I fix it for real.

Comment 3

[Wayne Mery (:wsmwk)]

close?

Comment 4

[:aceman]

Comment on attachment 8558631 [details] [diff] [review]
Use out-of-bounds value as EOF marker.

Review of attachment 8558631 [details] [diff] [review]:
-----------------------------------------------------------------

So you are changing the return values from char to int so you do not need to massage the -1 (failure) value into the 0..255 (or -126..127) range which all are valid characters.

Another approach would be to make the functions return a nsresult (which would indicate success or failure) and the char read would be in an out argument. Would converting to that be less readable?

If you do not want to try that version, please keep the EOF_CHAR constant as rkent requested that in bug 1085151. Just redefine its value to -1 . Or would e.g. -129 be safer for 'signed char' platforms?

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +389,5 @@
>    else
>    {
>      if (m_startWritingToBuffer)
>        m_unparsedFilterBuffer.Append(newChar);
> +    return (unsigned char)newChar;

Why forcing unsigned here?

Comment 5

[:aceman]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #3)
> close?

No, it is a valid proposal.

Comment 6

[Jorg K (CEST = GMT+2)]

Here
  https://hg.mozilla.org/comm-central/rev/273bcc4d6e9b
  Bug 1085151 - avoid char (-1) in FilterList to compile on powerpc, r=rkent, a=rkent 
they introduced |EOF_CHAR (char) 0xFF| and return it from a char-typed function to flag special events. Well, that in the first place is not real good since in general 0xFF is a valid character and not anything special. Anyway, apparently 0xFF doesn't seem to appear in the data, so maybe this idea is valid.

Which problem are we trying to fix here?
Compile error at
  do { } while (curChar != -1);
That code no longer exists, it reads while (curChar != EOF_CHAR); today.

Oh, this is the problem:
Bug 1085151 comment #11:
This is the wrong approach. 0xFF is a valid character (ÿ in ISO-8869-1, for example), and any attempt to use a character as an EOF marker is doomed to fail.

I agree with that, but how has it worked so far?

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8558631 [details] [diff] [review]
Use out-of-bounds value as EOF marker.

Review of attachment 8558631 [details] [diff] [review]:
-----------------------------------------------------------------

This is basically undoing bug 1085151 but changing the return type to int instead of char. This is better than defining a 0xFF EOF character. So f+.

Certainly f- for the way this bug was handled. Copied with nonsensical summary and description, as Aceman said in comment #1.

Oh, just reading #4: Yes, we could change EOF_CHAR to -1 but make the functions return int. That's less churn.

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +389,5 @@
>    else
>    {
>      if (m_startWritingToBuffer)
>        m_unparsedFilterBuffer.Append(newChar);
> +    return (unsigned char)newChar;

I don't understand this either. If nothing else, this should be cast to int.

@@ +450,4 @@
>  nsresult nsMsgFilterList::LoadValue(nsCString &value, nsIInputStream *aStream)
>  {
>    nsAutoCString  valueStr;
> +  int  curChar;

Fix two spaces here.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Use -1 as EOF marker instead of 0xFF

Like this, right?

Comment 9

[:aceman]

Comment on attachment 8803635 [details] [diff] [review]
Use -1 as EOF marker instead of 0xFF

Review of attachment 8803635 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this is what I meant.
But we need confirmation from the reporter if it still compiles on his platform.

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +471,3 @@
>        if (nextChar == '"')
>          curChar = '"';
>        else if (nextChar == '\\')  // replace "\\" with "\"

Are we sure this will work now with nextChar being int and comparing it to char ? But it should if it compares the numeric representation of the character.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #9)
> But we need confirmation from the reporter if it still compiles on his
> platform.
Sure, if he's still interested in the bug after a year.

> >        else if (nextChar == '\\')  // replace "\\" with "\"
> Are we sure this will work now with nextChar being int and comparing it to
> char ? But it should if it compares the numeric representation of the
> character.
Reading up on it here
http://stackoverflow.com/questions/22883451/comparing-char-to-int-in-c
it depends on whether the char is considered signed or unsigned.
For example, a character with the high bit set could be considered negative.

That's why the author had |return (unsigned char)newChar;| which we both didn't understand. This is needed to ensure that the character is considered as unsigned before up-casting to an int. We need to restore this.

I guess those single character literals are treated as the unsigned numeric value of the character.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Use -1 as EOF marker instead of 0xFF

Comment 12

[:aceman]

(In reply to Jorg K (GMT+2) from comment #10)
> > >        else if (nextChar == '\\')  // replace "\\" with "\"
> > Are we sure this will work now with nextChar being int and comparing it to
> > char ? But it should if it compares the numeric representation of the
> > character.
> Reading up on it here
> http://stackoverflow.com/questions/22883451/comparing-char-to-int-in-c
> it depends on whether the char is considered signed or unsigned.
> For example, a character with the high bit set could be considered negative.

And the whole point of this patch (and bug 1085151) is some problems with signed/unsigned chars on different platforms.

So we should wait on the reporter's check.

> That's why the author had |return (unsigned char)newChar;| which we both
> didn't understand. This is needed to ensure that the character is considered
> as unsigned before up-casting to an int. We need to restore this.

I had an issue with that, that the function was returning char (the conversion to int is just an extension of it), but suddenly we return forced unsigned char. Should we not let the signedness be determined by the platform automatically (via plain char)?

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #12)
> but suddenly we return forced
> unsigned char. Should we not let the signedness be determined by the
> platform automatically (via plain char)?

Now we return int and we want 0x80 to be retuned as 128 and not something negative. I'm sure the original patch author tried his patch. This is now his original patch, just with EOF_CHAR defined as -1.

Comment 14

[Andreas Schwab]

Comment on attachment 8803642 [details] [diff] [review]
Use -1 as EOF marker instead of 0xFF

https://build.opensuse.org/package/live_build_log/home:AndreasSchwab:f/seamonkey/p/ppc64

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/b48543077598fef7cb3577bf71405685a9305f86