Closed Bug 186547 Opened 17 years ago Closed 12 years ago

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

Categories

(MailNews Core :: MIME, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tenthumbs, Assigned: Bienvenu)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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)) { ... }.
Attached patch (Av1) simple patch (obsolete) — Splinter Review
This should do it.
Blocks: buildwarning
Attachment #109996 - Flags: review?(neil.parkwaycc.co.uk)
OS: Linux → All
Hardware: PC → All
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...)
Attachment #109996 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attachment #109996 - Attachment description: simple patch → (Av1) simple patch
Attachment #109996 - Attachment is obsolete: true
Attached patch (Av2) <comi18n.cpp> (obsolete) — Splinter Review
Is it better with |== 0| ?
If not, what would be a proper fix ?
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.
Attachment #137730 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch (Bv1) <prprf.*> (obsolete) — Splinter Review
Is the |if ((PRInt32)outlen == 0) {| block needed in |PR_snprintf()|,
since it is already in |PR_vsnprintf()| ?
Attachment #137730 - Flags: review?(neil.parkwaycc.co.uk) → review?(ducarroz)
Comment on attachment 137731 [details] [diff] [review]
(Bv1) <prprf.*>


'r=?': (see comment 5)
I have no compiler: Could you compile/test/review it ? Thanks.
Attachment #137731 - Flags: review?(cls)
Note that theses warnings are not currently listed in the "Blamed Build
Warnings" page.
Summary: Compiler warnings in mailnews/mime/src/comi18n.cpp → <comi18n.cpp>: Fix compiler build warnings. And update <prprf.*>.
Attachment #137731 - Flags: review?(cls) → review?(wchang0222)
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;
>+  }
Attachment #137731 - Flags: review?(wchang0222) → review-
Attached patch (Bv2) <prprf.c> (obsolete) — Splinter Review
Bv1, with comment 8 suggestion(s).
Comment on attachment 139064 [details] [diff] [review]
(Bv2) <prprf.c>


I have no compiler: Could you compile/test/review it ? Thanks.
Attachment #139064 - Flags: review?(wchang0222)
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>.
Attachment #137731 - Attachment is obsolete: true
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.
Attachment #139064 - Flags: review?(wchang0222) → review+
Attachment #139064 - Attachment is obsolete: true
Attachment #139076 - Flags: review?(darin)
Comment on attachment 137730 [details] [diff] [review]
(Av2) <comi18n.cpp>

ducarroz@aol.net wrote to me "I don't have a build environment setup right now!
Maybe David can do it?"
Attachment #137730 - Flags: review?(ducarroz) → review?(bienvenu)
Will PR_snprintf now return a detectable error value? Doesn't the latest patch 
really belong to bug 229662?
Comment on attachment 137730 [details] [diff] [review]
(Av2) <comi18n.cpp>


Re comment 15:

You're right:
This patch does depend on bug 229662...
Depends on: 229662
Attachment #139076 - Flags: review?(darin) → review+
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).
Attachment #139076 - Attachment description: (Bv3) <prprf.c> → (Bv3) <prprf.c> (checked in)
Attachment #139076 - Attachment description: (Bv3) <prprf.c> (checked in) → (Bv3) <prprf.c> [Checked in: Comment 17]
Attachment #139076 - Attachment is obsolete: true
Product: MailNews → Core
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...
Attachment #137730 - Flags: review?(bienvenu) → review-
Assignee: ducarroz → bienvenu
Attachment #137730 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #321443 - Flags: superreview?(neil)
Attachment #321443 - Flags: superreview?(neil) → superreview+
fixed, thanks for the original patch, Serge.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #139076 - Attachment is obsolete: false
Attachment #321443 - Attachment description: same idea, fix a call that wasn't checked at all. → (Av3) same idea, fix a call that wasn't checked at all. [Checked in: Comment 20]
(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.
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.