Bug 226743 – Reduce use of reinterpret_cast<> in the codebase

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Reduce use of reinterpret_cast<> in the codebase.

[Chris Dumez]

Created attachment 430779 [details]
Patch

[Chris Dumez]

Created attachment 430780 [details]
Patch

[Chris Dumez]

Created attachment 430784 [details]
Patch

[Chris Dumez]

Created attachment 430791 [details]
Patch

[Chris Dumez]

Created attachment 430803 [details]
Patch

[Darin Adler]

Comment on attachment 430803 [details]
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");

[Chris Dumez]

Created attachment 430842 [details]
Patch

[Chris Dumez]

Created attachment 430845 [details]
Patch

[EWS]

Committed r278619 (238604@main): <https://commits.webkit.org/238604@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430845 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79019798>