Fix -Wshadow warnings in mfbt/decimal

RESOLVED FIXED in Firefox 44

Status

()

Core
MFBT
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8665806 [details] [diff] [review]
Wshadow_mfbt-decimal.patch

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 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

3 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+
https://hg.mozilla.org/mozilla-central/rev/421057bb740c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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

2 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.
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

2 years ago
SGTM. Thanks.
You need to log in before you can comment on or make changes to this bug.