1018288 - Make -Wtype-limits fatal

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

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

Attachment: Patch v1

Steve, tagging you for the ARTPConnection.cpp change first.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8431657 [details] [diff] [review]
Patch v1

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

::: netwerk/protocol/rtsp/rtsp/ARTPConnection.cpp
@@ +144,5 @@
>      // rand() * 1000 may overflow int type, use long long.
>      unsigned start = (unsigned)((rand() * 1000ll) / RAND_MAX) + 15550;
>      start &= ~1;
>  
> +    for (uint16_t port = start; ; port += 2) {

I think Ethan should take a look at this one too. It looks to me like port may wraparound to 0, and I'm not sure that we want that.

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +1456,5 @@
>    uint32_t oldAccessCount = 0;
>    if (!aIsTemporary) {
>      oldAccessCount = mAccessCount;
>      mAccessCount -= mChildren[aIndex]->mAccessCount;
> +    // NS_ASSERTION(mAccessCount >= 0, "Invalid access count while updating!");

I don't think I should review this part of the patch :)

Comment 2

[Ethan Tseng [:ethan]]

Comment on attachment 8431657 [details] [diff] [review]
Patch v1

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

::: netwerk/protocol/rtsp/rtsp/ARTPConnection.cpp
@@ +144,5 @@
>      // rand() * 1000 may overflow int type, use long long.
>      unsigned start = (unsigned)((rand() * 1000ll) / RAND_MAX) + 15550;
>      start &= ~1;
>  
> +    for (uint16_t port = start; ; port += 2) {

Oh! I think you found a bug here.
Both the original and revised versions of this line are incorrect because the end conditions of this for loop are always true.
Instead of removing the end condition, I suggest to declare port as uint32_t, which could eliminate compile warning and be logically correct as well.

p.s. Actually, port was declared as "unsigned" initially. It was changed to "uint16_t" in the changeset 7770095ccedf from bug 996535. And I think it's just a careless mistake.

Comment 3

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

Thanks! Would you mind fixing that issue? I don't really want to make behaviour changes here :)

Comment 4

[Ethan Tseng [:ethan]]

Sure! I'd love to.
I will file a new bug to fix that issue and you can get rid of it in this bug. :)

Comment 5

[Ethan Tseng [:ethan]]

Just for the record, I filed bug 1020379 to track that issue.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Ethan!

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8431657 [details] [diff] [review]
Patch v1

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

Removing r? here. Ethan has a fix ready in Bug 1020379. I can review again if this doesn't fix things.

Comment 8

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

Attachment: Make -Wtype-limits fatal

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8469389 [details] [diff] [review]
Make -Wtype-limits fatal

Note that the C version of this is still a warning, not an error. Was that intentional? The other four warnings in this block match their C counterparts.

Comment 10

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

Yes, that's intentional. The warning is triggered by code of the form if (enum_value < ENUM_MIN) {} where ENUM_MIN is zero, and the enum is unsigned; particularly some upstream C code uses that pattern.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b2a1dd0a65

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to :Ms2ger from comment #10)
> Yes, that's intentional. The warning is triggered by code of the form if
> (enum_value < ENUM_MIN) {} where ENUM_MIN is zero, and the enum is unsigned;
> particularly some upstream C code uses that pattern.

Why not disable the warning for that third-party code?  We already do that in a number of places.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e5b2a1dd0a65

Comment 14

[Chris Peterson [:cpeterson]]

If we have known-but-wontfix warnings, we should suppress them in moz.build so (someday!) we can reach (actionable) zero warnings.

Comment 15

[Nathan Froyd [:froydnj]]

Comment 12 and comment 14.

Comment 16

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

Filed bug 1052014