Closed Bug 859128 Opened 8 years ago Closed 8 years ago

Code that produces negative values from SVGPathSegUtils::ArgCountForType should use signed arithmetic

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

Two of the warnings fixed in bug 859021 were:
{
e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/DOMSVGPathSegList.cpp(457) : warning C4146: unary minus operator applied to unsigned type, result still unsigned

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/DOMSVGPathSegList.cpp(532) : warning C4146: unary minus operator applied to unsigned type, result still unsigned
}

emk added "0 -" which silences the warning, but doesn't make the expression signed. I think we should probably just make the expression signed in the first place, by making ArgCountForType return a signed value (which is known to be nonnegative, but signed because we know we may end up negating it or passing derivatives of it as a signed value)
I found one other reason we should make 'argCount' and its derivatives signed.  We have:

> 390   uint32_t oldArgCount = SVGPathSegUtils::ArgCountForType(oldType);
> 391   uint32_t newArgCount = SVGPathSegUtils::ArgCountForType(domItem->Type());
[...]
> 409   uint32_t delta = newArgCount - oldArgCount;
> 410   if (delta != 0) {
> 411     for (uint32_t i = aIndex + 1; i < LengthNoFlush(); ++i) {
> 412       mItems[i].mInternalDataIndex += delta;

If newArgCount is less than oldArgCount there, then 'delta' will be on the order of UINT32_MAX, which is bizarre (it'll work, but it's pretty hacky & not a very good "delta" value).

If we make the argcount values (and the delta value) signed, then this will be saner.
I think this takes care of it; it builds locally (on linux), at least, and seems sane to me.  I'll give it a Try run before landing, to be sure it doesn't re-break the windows warning (or introduce another one) somehow.
Attachment #734403 - Flags: review?(jwatt)
Comment on attachment 734403 [details] [diff] [review]
fix v1 [incomplete]: make arg counts signed, since they're sometimes negated and/or subtracted from each other

er, forgot to include the SVGPathSegUtils changes. new patch w/ that coming soon.
Attachment #734403 - Flags: review?(jwatt)
Actually - this gets a bit messy if we unilaterally convert it to be signed, because we compare arg-counts to unsigned values in various places, and in virtually all the usages, an unsigned value is fine.

So instead, I think we should just cram the unsigned return value into a signed variable, in the few places where we need signed arithmetic.
Summary: Make SVGPathSegUtils::ArgCountForType return a signed value → Code that produces negative values from SVGPathSegUtils::ArgCountForType should use signed arithmetic
Attachment #734403 - Attachment description: fix v1: make arg counts signed, since they're sometimes negated and/or subtracted from each other → fix v1 [incomplete]: make arg counts signed, since they're sometimes negated and/or subtracted from each other
Attachment #734403 - Attachment is obsolete: true
This fixes the code from comment 0 and comment 1 that need to perform signed arithmetic with these arg-counts, with an explanatory comment.
Attachment #734884 - Flags: review?(jwatt)
(forgot to update commit message; fixed in this version)
Attachment #734884 - Attachment is obsolete: true
Attachment #734884 - Flags: review?(jwatt)
Attachment #734885 - Flags: review?(jwatt)
Attachment #734885 - Flags: review?(jwatt) → review+
Try run (builds only) as a sanity-check: https://tbpl.mozilla.org/?tree=Try&rev=28c7bbbf4db5
Status: NEW → ASSIGNED
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/421f600cc295
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.