620354 - variable "may be used uninitialized" warnings in TextRunWordCache::FinishTextRun

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Bug 609691 caused uninitialized variable warnings about wordStartsInsideCluster and wordStartsInsideLigature.

Comment 1

[Jonathan Kew [: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.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Sure, but my gcc doesn't realize that, and these would hide other, more useful warnings.

Comment 3

[Jonathan Kew [:jfkthame]]

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).

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 5

[Jonathan Kew [:jfkthame]]

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.

Comment 6

[Karl Tomlinson (: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.

Comment 7

[Karl Tomlinson (:karlt)]

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.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In that case, wontfix?

Comment 9

[Karl Tomlinson (:karlt)]

OK