Closed
Bug 1208357
Opened 9 years ago
Closed 9 years ago
Fix -Wshadow warnings in mfbt/decimal
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: cpeterson, Assigned: cpeterson)
Details
Attachments
(1 file)
11.22 KB,
patch
|
Ms2ger
:
review+
Waldo
:
superreview+
|
Details | Diff | Splinter Review |
and create fix-wshadow-warnings.patch for update.sh to patch upstream Decimal code.
mfbt/decimal/Decimal.cpp:127:9 [-Wshadow] declaration of 'high' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:127:9 [-Wshadow] declaration of 'low' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:232:69 [-Wshadow] declaration of 'formatClass' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:232:69 [-Wshadow] declaration of 'sign' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:240:80 [-Wshadow] declaration of 'coefficient' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:240:80 [-Wshadow] declaration of 'exponent' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:240:80 [-Wshadow] declaration of 'sign' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:282:63 [-Wshadow] declaration of 'exponent' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:282:63 [-Wshadow] declaration of 'sign' shadows a member of 'this'
mfbt/decimal/Decimal.cpp:487:14 [-Wshadow] declaration of 'remainder' shadows a member of 'this'
mfbt/decimal/Decimal.h:96:33 [-Wshadow] declaration of 'sign' shadows a member of 'this'
Attachment #8665806 -
Flags: review?(Ms2ger)
Comment 1•9 years ago
|
||
Comment on attachment 8665806 [details] [diff] [review]
Wshadow_mfbt-decimal.patch
Review of attachment 8665806 [details] [diff] [review]:
-----------------------------------------------------------------
This looks correct, but I want Waldo to sign off on it.
I don't guess they'd take it, but have you tried to submit this to upstream?
::: mfbt/decimal/Decimal.cpp
@@ +122,5 @@
>
> // This class is used for 128 bit unsigned integer arithmetic.
> class UInt128 {
> public:
> + UInt128(uint64_t aLot, uint64_t aHigh)
aLow
Attachment #8665806 -
Flags: superreview?(jwalden+bmo)
Attachment #8665806 -
Flags: review?(Ms2ger)
Attachment #8665806 -
Flags: review+
Comment 2•9 years ago
|
||
Comment on attachment 8665806 [details] [diff] [review]
Wshadow_mfbt-decimal.patch
Review of attachment 8665806 [details] [diff] [review]:
-----------------------------------------------------------------
Stupid aFoo style guide idiocy.
Attachment #8665806 -
Flags: superreview?(jwalden+bmo) → superreview+
Comment 4•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
![]() |
||
Comment 5•9 years ago
|
||
What does this buy us? The down side is that it makes pulling down upstream more of a pain (and the changes in this patch make it look like the compiler is stupid and it should be fixed if we're to pay attention to this warning).
Assignee | ||
Comment 6•9 years ago
|
||
I have been slowly fixing -Wshadow warnings in mozilla-central, but we're still a long way from being able to enable -Wshadow everywhere. I can revert this Decimal change, if you prefer.
I started to pull a more recent snapshot of upstream Decimal.cpp and reapply our in-tree patches, but there were some merge conflicts from upstream changes I didn't understand well enough to resolve confidently or caused some assertion failures. Also, the in-tree patches no longer unapply cleanly because they have been bitrotted by other changes made to our Decimal.h without patches.
![]() |
||
Comment 7•9 years ago
|
||
Yeah, I'm fixing those issues in bug 1245414 and bug 1245379.
I think for now I'll skip reapplying your patch when I sync up with upstream. I'm definitely open to discussing making this code -Wshadow friendly, but right now it seems that at least one compiler is being sufficiently dumb that it's causing us to make changes to the code that shouldn't be necessary. As Ms2ger mentions, it would also be preferable to make any changes upstream.
Assignee | ||
Comment 8•9 years ago
|
||
SGTM. Thanks.
You need to log in
before you can comment on or make changes to this bug.
Description
•