456150 - layout/ warnings sweep

Status: RESOLVED INCOMPLETE

(Core :: Layout, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: (rev 1) warnings patch

This fixes a lot of "easy" warnings in layout.  Many were uninitialized-variable warnings of the type where a human can see that there is no bug, but the compiler is not clever enough.  I generally prefer to squelch these, because I like not having to wade through false positives to find the build diagnostics that are my fault.

Warnings that remain are of three varieties: uninitialized-variable and signed/unsigned mismatch warnings where the surrounding code was too complicated for the correct fix to be obvious; complaints about enum mismatches, where the offending enums come from IDL headers, so if there's to be a fix it has to be in xpidlgen; and "this member function is masked by this other member function" warnings, where the fix would involve changing APIs.  There are, unfortunately, a lot of warnings in that category.

Comment 1

[Arpad Borsos [:Swatinem]]

Hm this bug looks like doing some of the same work I did in bug 452797 (which got r/sr+) and bug 454761.
However, looks like you have a better insight to how to correctly initialize uninitialized vars so I made this bug block my one.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: (rev 2) rediffed after bug 452797

Arpad Borsos got a lot of the unused-variable warnings in bug 452797, so this revision takes that out.  The reorganization of SyncFrameViewGeometryDependentProperties is still included as I think the code is quite a bit easier to comprehend this way.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: (rev 3) rediffed + more warnings squelched

This revision applies cleanly against rev 184ad4f909cd, and squelches yet more warnings.  With this and the patch in bug 458463 applied, the only warnings in layout/ with gcc 4.3 are:

 - the uninitialized fType warning described in bug 458473
 - one warning caused by the IDL problem described in bug 239460
 - a bunch of signed/unsigned comparison warnings in the table code, which
   wants to be a separate patch (dozens of things are signed that shouldn't be)
 - "method A::foo(...) is hidden by B::foo(...)" which I'm not even going to
   attempt.

Comment 4

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

I tend to prefer not to fix these uninitialized variable warnings; I'd rather gcc be noisy but valgrind be useful than have gcc be quite and make valgrind detect fewer real bugs.

I'd prefer to fix the CascadeSheetRulesInto warning the way I did in bug 458813 (I had those patches sitting in my tree for a bit).

Some of these are more than warning fixes; if you're going to do a big batch of warning fixes, it's probably best to separate the patches by what type of warning you're fixing.

Any chance you could split the SyncFrameViewGeometryDependentProperties change (which I assume includes the IsCanvasFrame stuff) into its own patch?

Comment 5

[Zack Weinberg (:zwol)]

(In reply to comment #4)
> I tend to prefer not to fix these uninitialized variable warnings; I'd rather
> gcc be noisy but valgrind be useful than have gcc be quite and make valgrind
> detect fewer real bugs.

I sympathize with this, but at the same time, I am unhappy with having to page through dozens of these to get to any actual failure in my build, and to remember which of them are not my fault.

I wonder if we can get an addition to valgrind.h to mark a variable initialization as only-to-silence-compiler.

> I'd prefer to fix the CascadeSheetRulesInto warning the way I did in bug 458813
> (I had those patches sitting in my tree for a bit).

Sure.  I see the first patch in that bug has been approved, do you plan to land it soon?

> Some of these are more than warning fixes; if you're going to do a big batch
> of warning fixes, it's probably best to separate the patches by what type of
> warning you're fixing.

Ok.  I can break this up in the next revision (probably not for a few days).

> Any chance you could split the SyncFrameViewGeometryDependentProperties change
> (which I assume includes the IsCanvasFrame stuff) into its own patch?

Definitely.  (And yes, it does.)  Do you want a separate bug too?

Comment 6

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

(In reply to comment #5)
> I wonder if we can get an addition to valgrind.h to mark a variable
> initialization as only-to-silence-compiler.

I'm not sure how such a thing would work; we don't do any special compilation for valgrind, as far as I know.

> > Any chance you could split the SyncFrameViewGeometryDependentProperties change
> > (which I assume includes the IsCanvasFrame stuff) into its own patch?
> 
> Definitely.  (And yes, it does.)  Do you want a separate bug too?

I think that would be better.

Comment 7

[Zack Weinberg (:zwol)]

I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.

Comment 8

[Zack Weinberg (:zwol)]

I don't think there's any point leaving this bug open. The patch is hopelessly bitrotted at this point and I have no intention of coming back to it.