[webkit-reviews] review granted: [Bug 226743] Reduce use of reinterpret_cast<> in the codebase : [Attachment 430803] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 226743: Reduce use of reinterpret_cast<> in the codebase
https://bugs.webkit.org/show_bug.cgi?id=226743

Attachment 430803: Patch

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




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

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

> Source/WTF/wtf/persistence/PersistentCoders.cpp:101
> +	   encoder.encodeFixedLengthData(string.characters8(), length *
sizeof(LChar));

sizeof(LChar) can really be left out

> Source/WTF/wtf/text/CString.h:65
> +    CString(const LChar* data, size_t length) :
CString(reinterpret_cast<const char*>(data), length) { }

This seems a little bit questionable. It’s only OK if we want a Latin-1 C
string, but typically we want UTF-8 C strings. The meaning of the "L" in
"LChar" is Latin-1.

On the other hand, it does "herd" all the reinterpret_cast calls into one place
by overloading for uint8_t as well as char. But I think using LChar is
misleading.

> Source/WTF/wtf/text/CString.h:75
> +    const LChar* characters8() const { return reinterpret_cast<const
LChar*>(data()); }

Same thought. The real characters8() function is always correct, if it can be
called it’s guaranteed to have Latin-1. This is more questionable. It can be
used to incorrectly treat UTF-8 as Latin-1.

So I would not have named this characters8() nor used the LChar type. Although
I suppose that’t correct if this CString happens to contain Latin-1. Maybe a
different name would be better that isn’t claiming the string is Latin-1. Maybe
we should teach CString  how to track, in debug versions at least or at compile
time, what encoding the characters are in, since we probably only use it for
UTF-8 and Latin-1, not all arbitrary other types.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:334
> +    constexpr char httpVersionStaticPreambleLiteral[] = "HTTP/";
> +    StringView httpVersionStaticPreamble(httpVersionStaticPreambleLiteral,
strlen(httpVersionStaticPreambleLiteral));
>      if (!httpStatusLine.startsWith(httpVersionStaticPreamble))
>	   return false;

This code seems like overkill. I would write this:

    constexpr char preamble[] = "HTTP/";
    if (!httpStatusLine.startsWith(preamble))
	return false;

    unsigned preambleLength = strlen(preamble);

Maybe someone thought they were optimizing by writing the other thing?

>
Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:535
>	       StringBuilder header;
> -	       header.appendCharacters(reinterpret_cast<const unsigned
char*>(CFDataGetBytePtr(webvttHeaderData)), length);
> +	       header.appendCharacters(CFDataGetBytePtr(webvttHeaderData),
length);
>	       header.append("\n\n");

The following would be more efficient than using StringBuilder:

    auto header = makeString(StringView { CFDataGetBytePtr(webvttHeaderData),
length }, "\n\n");