1245104 - Address some compilation maybe-uninitialized warnings

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Paul Bignier]

Attachment: 0001-Fixed-uninitialized-variables-warnings.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20160127060627

Steps to reproduce:

'./mach build' in the gecko repo


Actual results:

Got [-Wmaybe-uninitialized] warnings in layout


Expected results:

No warnings

Comment 1

[Daniel Holbert [:dholbert]]

Thanks for the patch! I have some concerns, though.

In particular: I'll bet these warnings are mostly (or entirely?) false positives -- the compiler simply can't tell that the "foo" usage is actually safe in a scenario like this:

  nscoord foo;
  if (ShouldUseFoo()) {
    foo = something;
  }
  [...]
  if (ShouldUseFoo()) {
    SomeFunction(foo);
  }

There are two problems with "fixing" problems of this sort:
 (1) Minor performance cost (useless setting of a variable that we don't need to set sometimes).

 (2) We lose the ability for tools to help us catch *real* uninitialized usages.  For example, suppose someone adds a buggy unguarded usage of foo to the code above (without a ShouldUseFoo() check):
  SomeOtherFunction(foo);
Right now, that would trigger a clang Wsometimes-uninitialized build warning, which we treat as an error, so we would absolutely catch that bug. (ASAN or valgrind would also help us find it.)  But if we take your patch's strategy & initialize |foo| to a bogus value (like 0) up-front, then the tools wouldn't be able to tell us that we're calling  SomeOtherFunction() on a meaningless bogus value here.

So: if this warning is causing noise, it might be better to simply disable this warning, instead of trying to "fix" all of its instances.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8714804 [details] [diff] [review]
0001-Fixed-uninitialized-variables-warnings.patch

I agree with dholbert.  This warning is typically not very helpful, imo...

Comment 3

[Daniel Holbert [:dholbert]]

So, I think this is WONTFIX (at least, with the current strategy of dummy-initializing these variables). Resolving as such.

If anyone's got a different proposal for addressing these in light of comment 1 here and bug 1225428 comment 18, we can reopen perhaps. (or probably better, file a new bug with discussion of the new strategy)