186547 - <comi18n.cpp>: Fix compiler build warnings. And update <prprf.*>.

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[tenthumbs]

comi18n.cpp:320: warning: comparison of unsigned expression < 0 is always false
comi18n.cpp:731: warning: comparison of unsigned expression < 0 is always false
comi18n.cpp:740: warning: comparison of unsigned expression < 0 is always false
comi18n.cpp:777: warning: comparison of unsigned expression < 0 is always false

In mailnews/mime/src/comi18n.cpp, all these warnings come from the same form

  if (PR_snprintf(...) < 0) { ... }

but PR_snprintf returns a PRUint32 hence the warnings. Gcc will optimize the 
conditional blocks out of existence. The eliminated blocks seem useful so 
this seems important.

The documented test (in prprf.h) is

  if (PR_snprintf(...) == ((PRUint32)-1)) { ... }.

Comment 1

[tenthumbs]

Attachment: (Av1) simple patch

This should do it.

Comment 2

[[e-mail]]

Comment on attachment 109996 [details] [diff] [review]
(Av1) simple patch

prprf.h lies - PR_snprintf can never return (PRUint32)-1, in fact it tries to
return as big a value as possible (although it can get it wrong...)

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <comi18n.cpp>

Is it better with |== 0| ?
If not, what would be a proper fix ?

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 137730 [details] [diff] [review]
(Av2) <comi18n.cpp>


'r=?': (see comment 3)
I have no compiler: Could you compile/test/review it ? Thanks.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <prprf.*>

Is the |if ((PRInt32)outlen == 0) {| block needed in |PR_snprintf()|,
since it is already in |PR_vsnprintf()| ?

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 137731 [details] [diff] [review]
(Bv1) <prprf.*>


'r=?': (see comment 5)
I have no compiler: Could you compile/test/review it ? Thanks.

Comment 7

[Serge Gautherie (:sgautherie)]

Note that theses warnings are not currently listed in the "Blamed Build
Warnings" page.

Comment 8

[Wan-Teh Chang]

Comment on attachment 137731 [details] [diff] [review]
(Bv1) <prprf.*>

Almost all the changes in this patch are incorrect
or unnecessary.

1. It is not useful to declare a PRUint32 parameter
as const because the parameter is passed by value,
so the function can only change its own copy and
won't affect the caller's copy.

2. Declaring the local variable 'rv' in PR_snprintf
as PRUint32 is the only good change in this patch.

3. You should follow the prevalent indentation style
in the file you modify.  The indentation level in
NSPR source files is four, not two.

4. Changes like the following are incorrect unless
the original code is incorrect.  You changed the
meaning of the comparisons, so if the original code
is correct, the new code will be wrong.  Also, the
location of the assertion should not be moved.

>-    PR_ASSERT((PRInt32)outlen > 0);
>-    if ((PRInt32)outlen <= 0) {
>-	return 0;
>-    }
>+  if ((PRInt32)outlen == 0) {
>+    PR_ASSERT((PRInt32)outlen != 0);
>+    return (PRUint32)0;
>+  }

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) <prprf.c>

Bv1, with comment 8 suggestion(s).

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 139064 [details] [diff] [review]
(Bv2) <prprf.c>


I have no compiler: Could you compile/test/review it ? Thanks.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 137731 [details] [diff] [review]
(Bv1) <prprf.*>


Re comment 8:

1- Right, yet, adding 'const' can't hurt: it makes it clear both to human and
compiler that the value is not intended to change...

2- (see Bv2)

3- My mistake: I didn't know (about NSPR).

4- (I won't argue)

5- Don't you want my 2 comment updates ?
<.h> is wrong;
<.c> is already and better in <.h>.

Comment 12

[Wan-Teh Chang]

Comment on attachment 139064 [details] [diff] [review]
(Bv2) <prprf.c>

r=wtc.	I'm going to attach a new patch that fixes
a similar problem.

Comment 13

[Wan-Teh Chang]

Attachment: (Bv3) <prprf.c> [Checked in: Comment 17]

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 137730 [details] [diff] [review]
(Av2) <comi18n.cpp>

[e-mail] wrote to me "I don't have a build environment setup right now!
Maybe David can do it?"

Comment 15

[tenthumbs]

Will PR_snprintf now return a detectable error value? Doesn't the latest patch 
really belong to bug 229662?

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 137730 [details] [diff] [review]
(Av2) <comi18n.cpp>


Re comment 15:

You're right:
This patch does depend on bug 229662...

Comment 17

[Wan-Teh Chang]

Comment on attachment 139076 [details] [diff] [review]
(Bv3) <prprf.c>
[Checked in: Comment 17]

Patch checked into the NSPR trunk (NSPR 4.5) and
the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.7 beta).

Comment 18

[David :Bienvenu]

Comment on attachment 137730 [details] [diff] [review]
(Av2) <comi18n.cpp>

I prefer !PR_snprintf, and I think you've missed a call. I'll attach a new patch...

Comment 19

[David :Bienvenu]

Attachment: (Av3) same idea, fix a call that wasn't checked at all. [Checked in: Comment 20]

Comment 20

[David :Bienvenu]

fixed, thanks for the original patch, Serge.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> Is the |if ((PRInt32)outlen == 0) {| block needed in |PR_snprintf()|,
> since it is already in |PR_vsnprintf()| ?

I filed bug 434397...

(In reply to comment #20)
> fixed, thanks for the original patch, Serge.

Thanks to tenthumbs for the initial patch too.