<div dir="ltr">Great. Thanks for the reviews, <a href="https://github.com/behdad/harfbuzz/pull/273/commits/8179ff5d7ba4a140cf6743729a22072800e98a79">done</a>.</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 27, 2016 at 3:01 AM, Nikolay Sivov <span dir="ltr"><<a href="mailto:bunglehead@gmail.com" target="_blank">bunglehead@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 27.06.2016 0:58, Ebrahim Byagowi wrote:<br>
> I had a chance to improve some minor thing around DirectWrite backend of<br>
> HarfBuzz on this pull request<br>
</span>> <<a href="https://github.com/behdad/harfbuzz/pull/273" rel="noreferrer" target="_blank">https://github.com/behdad/harfbuzz/pull/273</a>> which basically is<br>
<span class="">> suggestion of Nikolay reviews on this mail<br>
</span>> <<a href="https://lists.freedesktop.org/archives/harfbuzz/2015-September/005066.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/archives/harfbuzz/2015-September/005066.html</a>>.<br>
<span class="">> Thank you Nikolay for the suggestions and sorry for the delay :)<br>
<br>
</span>Hi, Ebrahim.<br>
<br>
You're very welcome, thanks for working on this.<br>
<br>
Another issue I see with current implementation (which is minor because<br>
it will allocate more than needed, not less, but still):<br>
<br>
> retry_getglyphs:<br>
> uint16_t* clusterMap = (uint16_t*) malloc (maxGlyphCount * sizeof (uint16_t));<br>
> uint16_t* glyphIndices = (uint16_t*) malloc (maxGlyphCount * sizeof (uint16_t));<br>
> DWRITE_SHAPING_TEXT_PROPERTIES* textProperties = (DWRITE_SHAPING_TEXT_PROPERTIES*)<br>
> malloc (maxGlyphCount * sizeof (DWRITE_SHAPING_TEXT_PROPERTIES));<br>
> DWRITE_SHAPING_GLYPH_PROPERTIES* glyphProperties = (DWRITE_SHAPING_GLYPH_PROPERTIES*)<br>
> malloc (maxGlyphCount * sizeof (DWRITE_SHAPING_GLYPH_PROPERTIES));<br>
<br>
Cluster map and text properties should use WCHAR text length, not glyph<br>
count.<br>
<br>
><br>
> hr = analyzer->GetGlyphs (textString, textLength, fontFace, FALSE,<br>
> isRightToLeft, &runHead->mScript, localeName, NULL, &dwFeatures,<br>
> featureRangeLengths, 1, maxGlyphCount, clusterMap, textProperties, glyphIndices,<br>
> glyphProperties, &glyphCount);<br>
><br>
> if (unlikely (hr == HRESULT_FROM_WIN32 (ERROR_INSUFFICIENT_BUFFER)))<br>
> {<br>
> free (clusterMap);<br>
> free (glyphIndices);<br>
> free (textProperties);<br>
> free (glyphProperties);<br>
><br>
> maxGlyphCount *= 2;<br>
><br>
> goto retry_getglyphs;<br>
> }<br>
<br>
And this could do realloc() or free/malloc on glyphIndices and<br>
glyphProperties only, because text length never changes.<br>
</blockquote></div><br></div>