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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

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)

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1 [incomplete]: make arg counts signed, since they're sometimes negated and/or subtracted from each other

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.

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2: Use a signed variable, in the callers that need signed arithmetic

This fixes the code from comment 0 and comment 1 that need to perform signed arithmetic with these arg-counts, with an explanatory comment.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2a: Use a signed variable, in the callers that need signed arithmetic

(forgot to update commit message; fixed in this version)

Comment 7

[Daniel Holbert [:dholbert]]

Try run (builds only) as a sanity-check: https://tbpl.mozilla.org/?tree=Try&rev=28c7bbbf4db5

Comment 8

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/421f600cc295

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/421f600cc295