Closed Bug 456150 Opened 11 years ago Closed 8 years ago

layout/ warnings sweep

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: zwol, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(1 file, 2 obsolete files)

Attached patch (rev 1) warnings patch (obsolete) — Splinter Review
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.
Attachment #339538 - Flags: review?(dbaron)
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
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.
Blocks: 454761
Depends on: 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.
Attachment #339538 - Attachment is obsolete: true
Attachment #339586 - Flags: superreview?(dbaron)
Attachment #339586 - Flags: review?(dbaron)
Attachment #339538 - Flags: review?(dbaron)
Depends on: 458473
Depends on: 458463
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.
Attachment #339586 - Attachment is obsolete: true
Attachment #341720 - Flags: superreview?(dbaron)
Attachment #341720 - Flags: review?(dbaron)
Attachment #339586 - Flags: superreview?(dbaron)
Attachment #339586 - Flags: review?(dbaron)
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?
(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?
(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.
Attachment #341720 - Flags: superreview?(dbaron)
Attachment #341720 - Flags: superreview-
Attachment #341720 - Flags: review?(dbaron)
Attachment #341720 - Flags: review-
I no longer work for Mozilla, I am deassigning myself from bugs I have no intention of working on as a volunteer.
Assignee: zackw → nobody
Status: ASSIGNED → NEW
Whiteboard: [build_warning]
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.