1208357 - Fix -Wshadow warnings in mfbt/decimal

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: 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'

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8665806 [details] [diff] [review]
Wshadow_mfbt-decimal.patch

Review of attachment 8665806 [details] [diff] [review]:
-----------------------------------------------------------------

Stupid aFoo style guide idiocy.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/421057bb740c

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/421057bb740c

Comment 5

[Jonathan Watt [:jwatt]]

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

Comment 6

[Chris Peterson [:cpeterson]]

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

[Jonathan Watt [:jwatt]]

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.

Comment 8

[Chris Peterson [:cpeterson]]

SGTM. Thanks.