[webkit-reviews] review denied: [Bug 172729] Fix build of Windows-specific code with ICU 59.1 : [Attachment 311591] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Konstantin Tokarev
<annulen at yandex.ru>'s request for review:
Bug 172729: Fix build of Windows-specific code with ICU 59.1
https://bugs.webkit.org/show_bug.cgi?id=172729

Attachment 311591: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 311591
  --> https://bugs.webkit.org/attachment.cgi?id=311591
Patch

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

It‘s fine to create these new function helpers as free functions; helps keep
the String class cleaner and contain fewer platform specifics. But note that
this is not the pattern we have used in WTF::String up until this point. We
have been using member functions for this sort of thing, with examples like the
String::createCFString function and the String::operator NSString * function.
But I think the explicit function approach is likely superior.

If we take this path, we should also remove the following functions, which I
believe were created for use on the Windows platform, and could be replaced by
the new functions:

    String::String(const UChar*), which takes a null-terminated 16-bit wide
string
    String::charactersWithNullTermination(), which creates a null-terminated
16-bit wide string

There’s nothing about these functions that makes them hard to implement in
non-member functions.

> Source/WTF/wtf/text/WCharStringExtras.h:36
> +inline wchar_t* stringToNullTerminatedWChar(const String& string)
> +{
> +    return
reinterpret_cast<wchar_t*>(string.charactersWithNullTermination().data());
> +}

This is wrong. This creates a vector, takes a pointer to its content, then
deletes that vector and returns the dangling pointer.