126484 - Occurences of uninitialized variables being used before being set (in Bidi files)

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Aleksey Nogin]

This bug is just for the warnings in layout/base/src/nsBidiPresUtils.cpp. Currently
(http://tinderbox.mozilla.org/SeaMonkey/warn1014136200.29882.html - Tue, 19 Feb
2002 11:30 EST) TBox shows the following warnings:

layout/base/src/nsBidiPresUtils.cpp:166
 `nsresult rv' might be used uninitialized in this function

layout/base/src/nsBidiPresUtils.cpp:222
 `PRInt32 contentOffset' might be used uninitialized in this function

layout/base/src/nsBidiPresUtils.cpp:228
 `PRBool isTextFrame' might be used uninitialized in this function

layout/base/src/nsBidiPresUtils.cpp:585
 `nscoord dWidth' might be used uninitialized in this function

P.S. See also bug 126457 that documents similar warnings in the files in the
intl/ directory - most of these warnings are also in *Bidi* files.

P.P.S. See also bug 94151 (now fixed) which documented similar warning in
nsBidiPresUtils.cpp

Comment 1

[Aleksey Nogin]

Bug 59652 is the meta-bug tracking the fight against these (potentially very
nasty) warnings.

P.S. Trying to make sure that 1.0 has as little warnings as possible.

Comment 2

[Aleksey Nogin]

OK, after some Bidi files were moved around, here are all the "xxx might be used
uninitialized" warnings I currently see in *Bidi*.cpp files:

content/shared/src/nsBidiUtils.cpp:239
 `PRInt8 leftNoTrJ' might be used uninitialized in this function

content/shared/src/nsBidiUtils.cpp:343
 `PRUint32 beginArabic' might be used uninitialized in this function

content/shared/src/nsBidiUtils.cpp:391
 `PRUint32 beginArabic' might be used uninitialized in this function

content/shared/src/nsBidiUtils.cpp:423
 `PRUint32 beginNumeral' might be used uninitialized in this function

layout/base/src/nsBidi.cpp:850
 `DirProp beforeNeutral' might be used uninitialized in this function

layout/base/src/nsBidiPresUtils.cpp:178
 `nsresult rv' might be used uninitialized in this function

layout/base/src/nsBidiPresUtils.cpp:234
 `PRInt32 contentOffset' might be used uninitialized in this function

layout/base/src/nsBidiPresUtils.cpp:240
 `PRBool isTextFrame' might be used uninitialized in this function

layout/base/src/nsBidiPresUtils.cpp:597
 `nscoord dWidth' might be used uninitialized in this function

Comment 3

[Aleksey Nogin]

P.S. Another "dangerous" warning in there:

- content/shared/src/nsBidiUtils.cpp:319 : Statement with no effect

Comment 4

[Aleksey Nogin]

Attachment: Shut up a few warnings.

This patch adds initialization for all the variables compiler was complaining
about. It seems that in most cases compiler was just not smart enough, but it's
probably worth fixing anyway (after all, according to bug 179819 such warning
ought to be considered compilation errors).

Comment 5

[Aleksey Nogin]

Comment on attachment 109600 [details] [diff] [review]
Shut up a few warnings.

Please review. Thanks!

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 109600 [details] [diff] [review]
Shut up a few warnings.

>-  PRInt8 leftJ, thisJ, rightJ;
>-  PRInt8 leftNoTrJ, rightNoTrJ;
>-  thisJ = eNJ;
>+  PRInt8 leftJ, thisJ = eNJ, rightJ;
>+  PRInt8 leftNoTrJ = eNJ, rightNoTrJ;

I find this style confusing. Please don't combine multiple declarations and
initializations on the same line, here and later in the patch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> It seems that in most cases compiler was just not smart enough, but it's
> probably worth fixing anyway

Simon, is any of this code performance-critical to the point where this
statement would be false (I sort of doubt it, but just checking).

Comment 8

[Aleksey Nogin]

Attachment: Shut up a few warnings, v2

Updated not to include several initializations in a variable declaration (per
comment #6). Is this better?

Comment 9

[Aleksey Nogin]

Comment on attachment 109600 [details] [diff] [review]
Shut up a few warnings.

Moving the review requests.

Comment 10

[Aleksey Nogin]

Comment on attachment 111676 [details] [diff] [review]
Shut up a few warnings, v2

Moving the review requests.

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 111676 [details] [diff] [review]
Shut up a few warnings, v2

r=smontagu, with an implied "no" to the question in comment 7 ;-)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111676 [details] [diff] [review]
Shut up a few warnings, v2

>-  PRInt32 firstRun, endRun, limitRun, runCount,
>-  temp, trailingWSStart=mTrailingWSStart;
>+  PRInt32 firstRun, endRun, limitRun, runCount, temp;

Isn't that _removing_ an initialization?

Comment 13

[Aleksey Nogin]

> >-  PRInt32 firstRun, endRun, limitRun, runCount,
> >-  temp, trailingWSStart=mTrailingWSStart;
> >+  PRInt32 firstRun, endRun, limitRun, runCount, temp;
>
> Isn't that _removing_ an initialization?

No, that's removing an unused variable.

Comment 14

[timeless]

do you need someone to commit this?

Comment 15

[Aleksey Nogin]

> do you need someone to commit this?

Yes, please!

Comment 16

[timeless]

done

Comment 17

[Aleksey Nogin]

Thanks!

V, all the Bidi warnings went away on brad TBox.