[webkit-reviews] review denied: [Bug 42759] Always freeing m_logAttrs in TextBreakIteratorGtk.cpp: setUpIterator() : [Attachment 92060] 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 92060: m_logAttrs freed when necessary
https://bugs.webkit.org/attachment.cgi?id=92060&action=review

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

Thanks for your contribution! What's your motivation for this change? Does it
provide a measurable performance improvement in breaking text? 

This patch is missing a ChangeLog entry, which is a requirement for all WebKit
patches. (see http://www.webkit.org/coding/contributing.html). I recommend
using the ./Tools/Scripts/webkit-patch tool to submit your patches. It will
warn you about style violations before you even submit your patch. You can also
use check-webkit-style. Here are the coding style guidelines:
http://www.webkit.org/coding/coding-style.html

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:194
> +    int m_logAttrsLen;

Should be m_logAttrsLength.

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:200
> +    , m_logAttrs(NULL)

In WebKit we use 0 instead of NULL except in situations where it would cause an
error or warning.

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

> +    }

Since you have only one statement in this block, the style guide says you must
omit the curly braces.