Closed Bug 1018288 Opened 5 years ago Closed 5 years ago

Make -Wtype-limits fatal

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch v1 (obsolete) — Splinter Review
Steve, tagging you for the ARTPConnection.cpp change first.
Attachment #8431657 - Flags: review?(sworkman)
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 :)
Attachment #8431657 - Flags: review?(sworkman)
Attachment #8431657 - Flags: review?(ettseng)
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.
Attachment #8431657 - Flags: review?(ettseng) → feedback+
Thanks! Would you mind fixing that issue? I don't really want to make behaviour changes here :)
Flags: needinfo?(ettseng)
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. :)
Flags: needinfo?(ettseng)
Just for the record, I filed bug 1020379 to track that issue.
Thanks Ethan!
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.
Attachment #8431657 - Flags: review?(sworkman)
Attachment #8469389 - Flags: review?(mshal)
Attachment #8431657 - Attachment is obsolete: true
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.
Attachment #8469389 - Flags: review?(mshal) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/e5b2a1dd0a65
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
If we have known-but-wontfix warnings, we should suppress them in moz.build so (someday!) we can reach (actionable) zero warnings.
Comment 12 and comment 14.
Flags: needinfo?(Ms2ger)
Depends on: 1052014
Filed bug 1052014
Flags: needinfo?(Ms2ger)
Depends on: 1055655
QA Whiteboard: [qa-]
Duplicate of this bug: 514107
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.