Closed
Bug 1125588
Opened 10 years ago
Closed 8 years ago
Code quality problems in various components
Categories
(Firefox Build System :: General, defect, P5)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: coolypf, Unassigned)
References
Details
Due to unified compilation being enabled by default, there are code quality problems in various components.
Reporter | ||
Updated•10 years ago
|
Component: DOM → Build Config
Comment 1•10 years ago
|
||
Yuan, what are the "code quality problems" specifically? Please include as much detail as you can. The list of blocking bugs in this bug seems like a random list of bugs that has nothing to do with unified builds.
Flags: needinfo?(coolypf)
Whiteboard: [INVALID?]
Comment 2•10 years ago
|
||
My interpretation is that Yuan is finding nonunified compilation errors, tracking them back to the bug that introduced them, attaching fixes to those bugs, and marking those bugs as blocking this one. (In other words, patching the problems in the original bugs and recording everything encountered in this metabug.)
I don't know if that's the right way to go about it or not, but if it is, we should probably rename this bug to "[meta] non-unified compilation errors" or "[meta] namespacing errors" or something.
I'm happy that Yuan is fixing them, though!
Comment 3•10 years ago
|
||
We probably need separate new bugs for the fixes, on the one-bug-per-problem basis.
Comment 4•10 years ago
|
||
Yes, we need separate bugs for the fixes. Thanks, Yuan!
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #1)
> Yuan, what are the "code quality problems" specifically? Please include as
> much detail as you can. The list of blocking bugs in this bug seems like a
> random list of bugs that has nothing to do with unified builds.
Here is the logic:
unified build enabled by default
.cpp/.c files merged together during compilation
#include and using namespace in a.cpp take effect in b.cpp
people forget adding #include and using namespace when modifying b.cpp since the compiler does not complain
the correctness of source code depends on the somewhat unstable unified build system
Flags: needinfo?(coolypf)
Reporter | ||
Comment 6•10 years ago
|
||
(In reply to Yuan Pengfei from comment #5)
> the correctness of source code depends on the somewhat unstable unified
> build system
Proof of unstableness:
Bug 1043701 Comment 7
Bug 1043701 Comment 8
Bug 1112945
Reporter | ||
Comment 7•10 years ago
|
||
(In reply to Steve Fink [:sfink, :s:] from comment #2)
> My interpretation is that Yuan is finding nonunified compilation errors,
> tracking them back to the bug that introduced them, attaching fixes to those
> bugs, and marking those bugs as blocking this one. (In other words, patching
> the problems in the original bugs and recording everything encountered in
> this metabug.)
>
> I don't know if that's the right way to go about it or not, but if it is, we
> should probably rename this bug to "[meta] non-unified compilation errors"
> or "[meta] namespacing errors" or something.
>
> I'm happy that Yuan is fixing them, though!
It seems that unified builds are also affected...
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #3)
> We probably need separate new bugs for the fixes, on the one-bug-per-problem
> basis.
Yeah.
I think the proper way is to file new bugs blocking the original ones as well as this one.
However, it is a little complicated for me to do that...
Bug 650161 -- blocked by Bug xxx \
Bug 1096089 -- blocked by Bug yyy -- blocking Bug 1125588
Bug 949703 -- blocked by Bug zzz /
Comment 9•10 years ago
|
||
(In reply to Yuan Pengfei from comment #8)
> (In reply to Boris Zbarsky [:bz] from comment #3)
> > We probably need separate new bugs for the fixes, on the one-bug-per-problem
> > basis.
>
> Yeah.
> I think the proper way is to file new bugs blocking the original ones as
> well as this one.
> However, it is a little complicated for me to do that...
>
> Bug 650161 -- blocked by Bug xxx \
> Bug 1096089 -- blocked by Bug yyy -- blocking Bug 1125588
> Bug 949703 -- blocked by Bug zzz /
No, you just need to make bug xxx for example block both bug 650161 and bug 1125588.
Updated•10 years ago
|
Whiteboard: [INVALID?]
Updated•10 years ago
|
Priority: -- → P2
Comment 10•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #3)
> We probably need separate new bugs for the fixes, on the one-bug-per-problem
> basis.
Alternatively, how about we just give these fixes blanket r+, and land them without bugs?
"No bug. Add missing header. r=unified"
or if you prefer a name, feel free to use
"No bug. Add missing header. r=sfink"
as long as it's just a header addition or namespace qualification.
Comment 11•10 years ago
|
||
(In reply to Steve Fink [:sfink, :s:] from comment #10)
> (In reply to Boris Zbarsky [:bz] from comment #3)
> > We probably need separate new bugs for the fixes, on the one-bug-per-problem
> > basis.
>
> Alternatively, how about we just give these fixes blanket r+, and land them
> without bugs?
>
> "No bug. Add missing header. r=unified"
>
> or if you prefer a name, feel free to use
>
> "No bug. Add missing header. r=sfink"
>
> as long as it's just a header addition or namespace qualification.
Might as well mention the bug that broke it: "Fixup for bug xxx. Add missing header. r=yyy"
Updated•10 years ago
|
Priority: P2 → P5
Comment 12•8 years ago
|
||
All the code quality problems in Firefox have been fixed!
But seriously, this looks like a tracking bug and all dependencies are closed. So resolving.
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•