Created attachment 498714 [details] [diff] [review] Patch v1 Bug 609691 caused uninitialized variable warnings about wordStartsInsideCluster and wordStartsInsideLigature.
Attachment #498714 - Flags: review?(jfkthame)
AFAICS, the warnings are incorrect. All uses of those variables are conditional on the aSuccessful flag; if it's true, they are initialized, and if it's false, they are never touched or read at all.
Sure, but my gcc doesn't realize that, and these would hide other, more useful warnings.
On the other hand, "initializing" them to fake values would prevent a smarter compiler (or a tool such as valgrind) correctly warning us if we were to introduce a codepath where they really are used when uninitialized. When reviewing bug 609691, Karl expressed a preference for doing it this way (see comment #2 there).
However, I believe it's much less likely that Valgrind will catch a bug here than that developers will just ignore all build warnings, some of which pointing to real bugs, due to the noise. Also, I'm expecting developers and reviewers to catch usage of those variables if they're not initialized correctly.
Comment on attachment 498714 [details] [diff] [review] Patch v1 Switching review to Karl - personally, I don't feel all that strongly one way or the other about this particular case, but I don't want to unilaterally reverse a decision that was explicitly brought up in previous review.
Attachment #498714 - Flags: review?(jfkthame) → review?(karlt)
> /home/karl/moz/dev/gfx/thebes/gfxTextRunWordCache.cpp:455: warning: 'wordStartsInsideCluster' may be used uninitialized in this function > /home/karl/moz/dev/gfx/thebes/gfxTextRunWordCache.cpp:456: warning: 'wordStartsInsideLigature' may be used uninitialized in this function These seem to merely indicate that gcc hasn't gone to the effort to work out whether or not this variable was used. The compiler could have worked out that these are never used uninitialized merely by analysing the flow of this single function.
Summary: Uninitialized variable warnings in TextRunWordCache::FinishTextRun → variable "may be used uninitialized" warnings in TextRunWordCache::FinishTextRun
Comment on attachment 498714 [details] [diff] [review] Patch v1 (In reply to comment #4) > However, I believe it's much less likely that Valgrind will catch a bug here > than that developers will just ignore all build warnings, some of which > pointing to real bugs, due to the noise. I don't agree that these warnings will cause developers to ignore all build warnings. There is already plenty of build spew and developers need to know what to look for. "may be used uninitialized" warnings are not as helpful as other warnings. Similar discussions have happened in bug 456150 and elsewhere. e.g. http://kerneltrap.org/node/6591 I'm usually in favour of changing syntax to suppress warnings, but not changing the generated instructions.
Attachment #498714 - Flags: review?(karlt) → review-
In that case, wontfix?
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.