nsSHistory.cpp:971:51: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

RESOLVED FIXED in mozilla20

Status

()

Core
Document Navigation
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Just noticed this build warning:
{
docshell/shistory/src/nsSHistory.cpp:971:51: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
}
for this code:
> 958   int32_t startSafeIndex = NS_MAX(0, aIndex - gHistoryMaxViewers);
> 959   int32_t endSafeIndex = NS_MIN(mLength, aIndex + gHistoryMaxViewers);
> [...]
> 971   for (uint32_t i = startSafeIndex; trans && i <= endSafeIndex; i++) {
> 972     nsCOMPtr<nsIContentViewer> viewer = GetContentViewerForTransaction(trans);
> 973     safeViewers.AppendObject(viewer);
> 974     nsISHTransaction *temp = trans;
> 975     temp->GetNext(getter_AddRefs(trans));
> 976   }

We have a loop with 'i' ranging from startSafeIndex to endSafeIndex.  Those xyzSafeIndex variables are both signed, so 'i' should be too.

(It looks like we're probably pretty sure that both xyzSafeIndex variables are nonnegative -- notably, "startSafeIndex" is initted with a NS_MAX(0,...) -- so this probably doesn't actually cause problems. Still, might as well declare the loop-variable w/ a type that matches the thing it's being intitialized from & compared to.)

We've had this issue since this cset:
 https://hg.mozilla.org/mozilla-central/rev/051b3d8ed545#l15.302
for bug 683777.
Created attachment 693994 [details] [diff] [review]
fix

This fixes it by making i an int32_t instead of uint32_t.

(Note that "i" is *only* used as a loop variable -- not as an array-index or a function-argument or anything -- so there shouldn't be any side effects from changing its type to match the others in the "for()" statement.)

Patch has 16 lines of context, to include the declarations of startSafeIndex & endSafeIndex, for clarity.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #693994 - Flags: review?(justin.lebar+bug)
Comment on attachment 693994 [details] [diff] [review]
fix

Thanks for the extra context!
Attachment #693994 - Flags: review?(justin.lebar+bug) → review+

Comment 4

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1a490822a502
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.