Just noticed this build warning in an opt build log: > content/svg/content/src/SVGPathData.cpp:261:12: warning: > 'segType' may be used uninitialized in this function AFAICT, this is a legit warning. Code snippet for context: > 261 uint32_t segType, prevSegType = nsIDOMSVGPathSeg::PATHSEG_UNKNOWN; [...] > 273 while (i < mData.Length()) { > 274 segType = SVGPathSegUtils::DecodeType(mData[i++]); [...] > 483 prevSegType = segType; > 484 segStart = segEnd; > 485 } [...] > 489 MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS; https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGPathData.cpp#261 and that macro at the end is defined as: > 240 #define MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS \ > 241 do { \ > 242 if (capsAreSquare && !subpathHasLength && subpathContainsNonArc && \ > 243 SVGPathSegUtils::IsValidType(prevSegType) && \ > 244 (!IsMoveto(prevSegType) || \ > 245 segType == nsIDOMSVGPathSeg::PATHSEG_CLOSEPATH)) { \ > 246 ApproximateZeroLengthSubpathSquareCaps(segStart, aCtx); \ > 247 } \ > 248 } while(0) https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGPathData.cpp#240 So -- if mData has zero length, we'll never initialize segType, but we go on to read it when we evaulate the macro. Also -- incidentally, there's no reason to use "do { ... } while(0)" there, AFAICT -- we could just as easily use "{ ... }"

(In reply to Daniel Holbert [:dholbert] from comment #0) > Also -- incidentally, there's no reason to use "do { ... } while(0)" there, > AFAICT -- we could just as easily use "{ ... }" (sorry, disregard that editorializing -- apparently this is necessary for it to work as a macro, since do{ }while() is a statement whereas {} is a block)

Also: MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS seems to be making a distinction between segType and prevSegType, and in most of the places where it's invoked, they are indeed distinct. However, when the macro is invoked at line 489 (the spot quoted in comment 0), segType and prevSegType are guaranteed to be the same (disregarding the segtype-was-never-initialized possibility), because we set "prevSegType = segType" back on line 483 and then completed the loop. Is that bad?

Here's a patch to initialize |segType|, fixing this bug. It also adds an assertion after the loop to make it clear that segType and prevSegType are always the same at that point. (though I'm not actually sure if that's what the MAYBE_* function wants, per prev. comment.) Try run (green): https://tbpl.mozilla.org/?tree=Try&rev=7704f3292691

(In reply to Daniel Holbert [:dholbert] from comment #0) > So -- if mData has zero length Ah, but the function starts thus: 253 if (!mData.Length() || !IsMoveto(SVGPathSegUtils::DecodeType(mData[0]))) { 254 return; // paths without an initial moveto are invalid 255 } (Not that I disagree that it's a good idea to make the change.)

Ah, nice -- so we're safe after all. (I thought I checked for that, but I suppose I skimmed too quickly. IIRC I was looking for a comparison to 0 or an "Empty" check, and overlooked the Length()) I still think it's worth initializing segType, in any case -- it's essentially free, it fixes a build warning, and it makes this code a little more future-proof. Here's a new patch-version, which also changes the "if (!mData.Length()) to "if (mData.IsEmpty())" to make the empty-array-check more explicit / more readable (IMHO at least). We've got an IsEmpty method, so we might as well use it. :)

Thanks! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6337d6dd9727