293739 - PRBool success might be used uninitialized ... (nsBulletFrame line 1114)

Status: RESOLVED DUPLICATE of bug 534171

(Core :: Layout, defect)

Description

[Ben Fowler]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.7) Gecko/20050418 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.7) Gecko/20050418 Firefox/1.0.3

At and below  
http://lxr.mozilla.org/mozilla/source/layout/generic/nsBulletFrame.cpp#1114
if any of the first four switch branches are taken, then indeed 'success'
might be used without being assigned to.

Maybe this can't happen.

There may be more than one way of fixing this warning - would anyone like a 
patch?

Reproducible: Always

Comment 1

[Justin Wood (:Callek)]

Confirming, if you are willing to make a patch please do.

I would suggest pre-setting success to PR_True rather than mucking with those
first 4 case's.  Though if you have another idea, go for it. [Please note I am
not a reviewer]

Comment 2

[Ben Fowler]

Attachment: Patch to fix compiler warning, by ensuring that success is always initialised

Add initalisation of sucess to PR_TRUE, and some comments. PR_TRUE
(requested conversion succeeded) is the default condition, PR_FALSE
implies that some fall-back operation took place.

Comment 3

[Justin Wood (:Callek)]

I would suggest requesting review from a particular person, (preferrably one
with a shorter review que)

Use
http://www.mozilla.org/owners.html  if you are unsure.

Comment 4

[Ben Fowler]

Thank you. It might be David Baron, but I don't know enough about requesting 
review and review queues to jump in!

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

generally documentation for functions is put in the header file...

Comment 6

[Justin Wood (:Callek)]

Comment on attachment 183580 [details] [diff] [review]
Patch to fix compiler warning, by ensuring that success is always initialised

Targetting review and adding sr? since this should be a simple fix; Though I am
with biesi on that the documentation section should be in the class
declaration, not in its implimentation.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 183580 [details] [diff] [review]
Patch to fix compiler warning, by ensuring that success is always initialised

The massive comment at the beginning of AppendCounterText is way too verbose,
and probably belongs in the header file anyway.  Other than the @param lines,
most of it is either unnecessary or incorrect.

The comment about retrying is also incorrect.  It's not clear why the boolean
return exists at all, but it's not for retrying, since the function can return
false even when it does append text to the string.

Comment 8

[Ben Fowler]

Attachment: Improved patch

Shortened comment and removed comment, as above

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ben, you want to request review from a specific person (see
http://www.mozilla.org/owners.html for the relevant module owners), not review
in general...

Comment 10

[Ben Fowler]

Comment on attachment 183893 [details] [diff] [review]
Improved patch

Requesting review from dbaron. Sorry for the hold-up

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 183893 [details] [diff] [review]
Improved patch

+ * @param result The nsString to be appended to
+  **/

wrong indentation on this second line, and is there a reason you did not put
the comment in the header file?

Comment 12

[Ben Fowler]

Attachment: Removed comment

White space change was probably an editing error. I have removed the
comment altogether - it is obviously out of place

Comment 13

[Matthias Versen [:Matti]]

This patch got review but got never checked in.
Do we need an updated patch or is this now obsolete since the PRBool-> Bool switch ?