[webkit-reviews] review denied: [Bug 42759] Always freeing m_logAttrs in TextBreakIteratorGtk.cpp: setUpIterator() : [Attachment 92237] m_logAttrs freed when necessary

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Peter Hatina
<phatina at redhat.com>'s request for review:
Bug 42759: Always freeing m_logAttrs in TextBreakIteratorGtk.cpp:
setUpIterator()
https://bugs.webkit.org/show_bug.cgi?id=42759

Attachment 92237: m_logAttrs freed when necessary
https://bugs.webkit.org/attachment.cgi?id=92237&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92237&action=review

Seems okay. But really m_logAttrs should be a GOwnPtr if possible.

> Source/WebCore/ChangeLog:17
> +	   When breaking the text, I think, it is not always necessary to
reallocate the memory
> +	   block, if it is sufficient for future use. Also we save a mem.
allocation call and part
> +	   of the zeroing proces, which is done only on the part of the
portion. To my mind,
> +	   it is also cheaper to use once-prepared block, than always release
it and call
> +	   for another one.

Feel free to just say "Avoid an unecessary memory allocation, if we can." or
something like that. :)

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:193
>      PangoLogAttr* m_logAttrs;

Might as well change this to a GOwnPtr<PangoLogAttr> now. You can do away with
all calls to g_free then. Right now it looks like m_logAttrs just leaks!

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:205
> +TextBreakIterator::TextBreakIterator()
> +    : m_type(UBRK_CHARACTER)
> +    , m_logAttrs(0)
> +    , m_logAttrsLength(0)
> +    , m_charIterator()
> +{
> +}
> +

Hrm. I believe you should either pass these parameters to the constructor and
itialize them or initialize them externally. Not both though.

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:231
> +	   iterator->m_logAttrs = g_new0(PangoLogAttr, charLength + 1);
> +	   iterator->m_logAttrsLength = charLength + 1;
> +    } else
> +	   memset(static_cast<void*>(iterator->m_logAttrs), 0, charLength + 1);

> +

Since you always need to do memset you can simplify this:

if (createdIterator) {
    iterator->m_logAttrs = g_new(PangoLogAttr, charLength + 1);
    iterator->m_logAttrsLength = charLength + 1;
}
memset(static_cast<void*>(iterator->m_logAttrs), 0, charLength + 1);

I'm still curious whether this has any measurable performance ramifications.