[Webkit-unassigned] [Bug 42759] Always freeing m_logAttrs in TextBreakIteratorGtk.cpp: setUpIterator()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42759


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92237|review?                     |review-
               Flag|                            |




--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2011-05-04 15:30:37 PST ---
(From update of attachment 92237)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.