[webkit-reviews] review granted: [Bug 227449] ANGLE Metal primitive restart range computation could index with size_t : [Attachment 432394] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 227449: ANGLE Metal primitive restart range computation could index with
size_t
https://bugs.webkit.org/show_bug.cgi?id=227449

Attachment 432394: Patch

https://bugs.webkit.org/attachment.cgi?id=432394&action=review




--- Comment #4 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 432394
  --> https://bugs.webkit.org/attachment.cgi?id=432394
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432394&action=review

This is certainly easier to understand. r+ with a couple of small questions.

I defer to kpiddington's review if there's any further feedback.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:421
> +    for (size_t i = 0; i < numIndices; i++)

Would it be better to use "++i" here for consistency with the inner loop?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:430
> +	   } while (i < numIndices && bufferData[i] == restartMarker);

Out of curiosity do you want to leave the restart indices in these ranges or
elide them?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/BufferMtl.mm:445
> +		   mRestartIndices = calculateRestartRanges<uint8_t>(ctx,
getCurrentBuffer());

How hot is this routine? If it's called very often then maybe there is an
advantage to continuing to pass mRestartIndices as an argument to reduce
copying (or is return value optimization expected to handle this)?