Bug 226623 - Use Vector<uint8_t> instead of Vector<char> to store bytes in SharedBuffer
Summary: Use Vector<uint8_t> instead of Vector<char> to store bytes in SharedBuffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-03 21:38 PDT by Chris Dumez
Modified: 2021-06-04 21:08 PDT (History)
44 users (show)

See Also:


Attachments
Patch (122.84 KB, patch)
2021-06-03 21:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (125.30 KB, patch)
2021-06-03 22:54 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (127.07 KB, patch)
2021-06-03 22:59 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (129.52 KB, patch)
2021-06-03 23:06 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (130.28 KB, patch)
2021-06-03 23:14 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (130.70 KB, patch)
2021-06-03 23:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (138.18 KB, patch)
2021-06-04 07:38 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (140.33 KB, patch)
2021-06-04 07:45 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (141.83 KB, patch)
2021-06-04 08:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (254.31 KB, patch)
2021-06-04 14:32 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (252.40 KB, patch)
2021-06-04 14:43 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (253.86 KB, patch)
2021-06-04 14:50 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (254.11 KB, patch)
2021-06-04 15:01 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (257.72 KB, patch)
2021-06-04 15:26 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (258.64 KB, patch)
2021-06-04 15:32 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (259.57 KB, patch)
2021-06-04 15:43 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (261.83 KB, patch)
2021-06-04 16:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-06-03 21:38:42 PDT
Use Vector<uint8_t> instead of Vector<char> to store bytes in SharedBuffer. Also have SharedBuffer::data() return a `const uint8_t*` instead of `const char*`.
This is our preferred type to store bytes.
Comment 1 Chris Dumez 2021-06-03 21:45:00 PDT
Created attachment 430537 [details]
Patch
Comment 2 Chris Dumez 2021-06-03 22:54:15 PDT
Created attachment 430542 [details]
Patch
Comment 3 Chris Dumez 2021-06-03 22:59:05 PDT
Created attachment 430543 [details]
Patch
Comment 4 Chris Dumez 2021-06-03 23:06:59 PDT
Created attachment 430545 [details]
Patch
Comment 5 Chris Dumez 2021-06-03 23:14:49 PDT
Created attachment 430549 [details]
Patch
Comment 6 Chris Dumez 2021-06-03 23:27:01 PDT
Created attachment 430552 [details]
Patch
Comment 7 Chris Dumez 2021-06-04 07:38:18 PDT
Created attachment 430575 [details]
Patch
Comment 8 Chris Dumez 2021-06-04 07:45:13 PDT
Created attachment 430576 [details]
Patch
Comment 9 Chris Dumez 2021-06-04 08:06:49 PDT
Created attachment 430577 [details]
Patch
Comment 10 Darin Adler 2021-06-04 11:04:44 PDT
Comment on attachment 430577 [details]
Patch

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

We may want to work harder to make sure continue to adopt vectors in cases where this patch changes us to copy instead.

We may also want to use const void* and void* more on functions that take and process bytes, rather than using a specific byte pointer types.

> Source/WTF/wtf/FileSystem.h:156
> +WTF_EXPORT_PRIVATE int writeToFile(PlatformFileHandle, const char* data, int length); // FIXME: Should use `const uint8_t*`.

Maybe it should take const void* like the POSIX write() function.

> Source/WTF/wtf/FileSystem.h:158
> +WTF_EXPORT_PRIVATE int readFromFile(PlatformFileHandle, char* data, int length); // FIXME: Should use `uint8_t*`.

Maybe it should take void* like the POSIX read() function.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:197
> +        const char* nextBoundary = static_cast<const char*>(memmem(currentBoundary + boundaryLength, length - (currentBoundary + boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:203
> +            nextBoundary = static_cast<const char*>(memmem(nextBoundary + boundaryLength, length - (nextBoundary + boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));

Ditto.

> Source/WebCore/Modules/highlight/AppHighlight.h:55
> +    encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(highlight->data()), highlight->size(), 1);

Why is a cast needed here? Doesn’t data() return a uint8_t*?

> Source/WebCore/Modules/indexeddb/IDBGetResult.cpp:37
> +    m_value = ThreadSafeDataBuffer::create({ buffer.data(), buffer.size() });

No cleaner idiom? Maybe a std::span or Vector?

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:172
> +        const uint8_t* data = reinterpret_cast<const uint8_t*>(buffer->data.data<uint8_t>());

auto would be better here.

Why is reinterpret_cast needed here? What type does data<uint8_t>() return? Why not uint8_t*?

> Source/WebCore/css/CSSFontFaceSource.cpp:168
> -                    m_generatedOTFBuffer = SharedBuffer::create(WTFMove(otfFont.value()));
> +                    m_generatedOTFBuffer = SharedBuffer::create(otfFont->data(), otfFont->size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/fileapi/Blob.cpp:117
> +    blobParts.append(Vector { buffer.data(), buffer.size() });

This seems to call for std::span. Or doesn’t SharedBuffer have a function that returns a Vector?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:583
> +        NetworkResourcesData::ResourceData const* resourceData = m_resourcesData->maybeAddResourceData(requestId, reinterpret_cast<const uint8_t*>(data), dataLength);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:159
> +        m_client->didReceiveData(this, buffer ? buffer->data() : reinterpret_cast<const uint8_t*>(data), buffer ? buffer->size() : length);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:157
> -        content->append(WTFMove(part));
> +        content->append(part.data(), part.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:198
> +        auto decodedData = base64Decode(content->data()   , content->size());

Should probably get rid of the extra spaces here.

> Source/WebCore/loader/cache/CachedSVGFont.cpp:92
> -            m_convertedFont = SharedBuffer::create(WTFMove(convertedFont.value()));
> +            m_convertedFont = SharedBuffer::create(convertedFont->data(), convertedFont->size());

Is this changing from adopting data to copying it, or did the old version already copy the data?

> Source/WebCore/platform/SharedBuffer.cpp:142
> -    auto arrayBuffer = ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()), sizeof(char));
> +    auto arrayBuffer = ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()), sizeof(uint8_t));

Not new: What guarantees size() fits in unsigned?

Both of these sizeof expression are silly ways to write the number 1. I think we could just write 1.

> Source/WebCore/platform/SharedBuffer.cpp:232
> +        [](const RetainPtr<CFDataRef>& data) { return reinterpret_cast<const uint8_t*>(CFDataGetBytePtr(data.get())); },

No typecast needed any more: CFDataGetBytePtr returns const uint8_t*.

> Source/WebCore/platform/SharedBuffer.cpp:235
> +        [](const GRefPtr<GBytes>& data) { return reinterpret_cast<const uint8_t*>(g_bytes_get_data(data.get(), nullptr)); },

This should be static_cast, not reinterpret_cast; it’s a cast from a void*.

> Source/WebCore/platform/SharedBuffer.cpp:238
> +        [](const RefPtr<GstMappedOwnedBuffer>& data) { return reinterpret_cast<const uint8_t*>(data->data()); },

No typecast needed any more: GstMappedOwnedBuffer::data returns const uint8_t*.

> Source/WebCore/platform/SharedBuffer.cpp:240
> +        [](const FileSystem::MappedFileData& data) { return reinterpret_cast<const uint8_t*>(data.data()); }

This should be static_cast, not reinterpret_cast; it’s a cast from a void*.

> Source/WebCore/platform/SharedBuffer.cpp:357
> +    char* p = reinterpret_cast<char*>(buffer.data());

Always a sad moment when we have to add a reinterpret_cast; any way to avoid this one?

> Source/WebCore/platform/SharedBuffer.cpp:370
> +    buffer.shrink(p - reinterpret_cast<char*>(buffer.data()));

Always a sad moment when we have to add a reinterpret_cast; any way to avoid this one?

> Source/WebCore/platform/SharedBufferChunkReader.cpp:103
> +                chunk.append(reinterpret_cast<const uint8_t*>(m_separator.data()), m_separatorIndex);

Always a sad moment when we have to add a reinterpret_cast; any way to avoid this one?

> Source/WebCore/platform/graphics/WOFFFileFormat.cpp:297
> -        buffer = SharedBuffer::create(WTFMove(convertedFont));
> +        buffer = SharedBuffer::create(convertedFont.data(), convertedFont.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:239
> +    const uint8_t* source = sharedBuffer->data() + position;

auto

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:89
> +        size_t offset = reinterpret_cast<const uint8_t*>(position) - buffer.data();

This can be static_cast instead of reinterpret_cast since it’s from a void*.

> Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:318
> +    return strncmp(&m_data->dataAsCharPtr()[imageOffset], "\x89PNG", 4) ? BMP : PNG;

This should use memcmp instead of strncmp--it was never useful to use strncmp here, no reason to handle null characters specially--and then we will not need dataAsCharPtr here.

> Source/WebCore/platform/network/FormData.cpp:417
> +    auto bytes = flatten();
> +    return SharedBuffer::create(bytes.data(), bytes.size());

I think this is now copying instead of adopting the Vector. That’s not as good.

> Source/WebCore/platform/win/SharedBufferWin.cpp:55
> +        if (ReadFile(fileHandle, (LPVOID)buffer.data(), bytesToRead, &bytesRead, 0) && bytesToRead == bytesRead)

Why was this new typecast needed?

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:362
> -    OffsetBuffer(Vector<char> buffer)
> +    OffsetBuffer(Vector<uint8_t>&& buffer)

Nice optimization.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:364
>          , m_currentOffset(0)

We should move this initialization down to where the data member is defined below.

> Source/WebKit/WebProcess/Plugins/Netscape/NetscapePluginStream.cpp:120
> +    deliverData(reinterpret_cast<const uint8_t*>(resultCString.data()), resultCString.length());

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1172
> +    m_pluginView->manualLoadDidReceiveData(reinterpret_cast<const uint8_t*>(data), length);

Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?
Comment 11 Chris Dumez 2021-06-04 12:16:35 PDT
(In reply to Darin Adler from comment #10)
> Comment on attachment 430577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430577&action=review
> 
> We may want to work harder to make sure continue to adopt vectors in cases
> where this patch changes us to copy instead.

Yes, I am definitely planning to follow-up to fix any case that got worse.

> 
> We may also want to use const void* and void* more on functions that take
> and process bytes, rather than using a specific byte pointer types.

Yes, I think that's a good idea.

> 
> > Source/WTF/wtf/FileSystem.h:156
> > +WTF_EXPORT_PRIVATE int writeToFile(PlatformFileHandle, const char* data, int length); // FIXME: Should use `const uint8_t*`.
> 
> Maybe it should take const void* like the POSIX write() function.

Doing this one in this patch since it is a significant improvement at call sites.

> > Source/WTF/wtf/FileSystem.h:158
> > +WTF_EXPORT_PRIVATE int readFromFile(PlatformFileHandle, char* data, int length); // FIXME: Should use `uint8_t*`.
> 
> Maybe it should take void* like the POSIX read() function.

Agree, will add a FIXME comment and address in a follow-up.

> 
> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:197
> > +        const char* nextBoundary = static_cast<const char*>(memmem(currentBoundary + boundaryLength, length - (currentBoundary + boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?
> 
> > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:203
> > +            nextBoundary = static_cast<const char*>(memmem(nextBoundary + boundaryLength, length - (nextBoundary + boundaryLength - reinterpret_cast<const char*>(data)), boundary.data(), boundaryLength));
> 
> Ditto.

Will check.

> 
> > Source/WebCore/Modules/highlight/AppHighlight.h:55
> > +    encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(highlight->data()), highlight->size(), 1);
> 
> Why is a cast needed here? Doesn’t data() return a uint8_t*?

Will drop.

> 
> > Source/WebCore/Modules/indexeddb/IDBGetResult.cpp:37
> > +    m_value = ThreadSafeDataBuffer::create({ buffer.data(), buffer.size() });
> 
> No cleaner idiom? Maybe a std::span or Vector?
> 
> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:172
> > +        const uint8_t* data = reinterpret_cast<const uint8_t*>(buffer->data.data<uint8_t>());
> 
> auto would be better here.
> 
> Why is reinterpret_cast needed here? What type does data<uint8_t>() return?
> Why not uint8_t*?

Will use auto and drop the cast.

> 
> > Source/WebCore/css/CSSFontFaceSource.cpp:168
> > -                    m_generatedOTFBuffer = SharedBuffer::create(WTFMove(otfFont.value()));
> > +                    m_generatedOTFBuffer = SharedBuffer::create(otfFont->data(), otfFont->size());
> 
> I think this is now copying instead of adopting the Vector. That’s not as
> good.

It is. I am planning to look into this in a follow-up.

> > Source/WebCore/fileapi/Blob.cpp:117
> > +    blobParts.append(Vector { buffer.data(), buffer.size() });
> 
> This seems to call for std::span. Or doesn’t SharedBuffer have a function
> that returns a Vector?

AFAIK, std::span is not a thing YET in WebKit :) SharedBuffer doesn't have a function that returns a Vector at the moment.

> 
> > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:583
> > +        NetworkResourcesData::ResourceData const* resourceData = m_resourcesData->maybeAddResourceData(requestId, reinterpret_cast<const uint8_t*>(data), dataLength);
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?
> 
> > Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:159
> > +        m_client->didReceiveData(this, buffer ? buffer->data() : reinterpret_cast<const uint8_t*>(data), buffer ? buffer->size() : length);
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

Will check.

> 
> > Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:157
> > -        content->append(WTFMove(part));
> > +        content->append(part.data(), part.size());
> 
> I think this is now copying instead of adopting the Vector. That’s not as
> good.
> 
> > Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:198
> > +        auto decodedData = base64Decode(content->data()   , content->size());
> 
> Should probably get rid of the extra spaces here.

Will fix.

> 
> > Source/WebCore/loader/cache/CachedSVGFont.cpp:92
> > -            m_convertedFont = SharedBuffer::create(WTFMove(convertedFont.value()));
> > +            m_convertedFont = SharedBuffer::create(convertedFont->data(), convertedFont->size());
> 
> Is this changing from adopting data to copying it, or did the old version
> already copy the data?
> 
> > Source/WebCore/platform/SharedBuffer.cpp:142
> > -    auto arrayBuffer = ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()), sizeof(char));
> > +    auto arrayBuffer = ArrayBuffer::tryCreateUninitialized(static_cast<unsigned>(size()), sizeof(uint8_t));
> 
> Not new: What guarantees size() fits in unsigned?
> 
> Both of these sizeof expression are silly ways to write the number 1. I
> think we could just write 1.

Will use 1.

> 
> > Source/WebCore/platform/SharedBuffer.cpp:232
> > +        [](const RetainPtr<CFDataRef>& data) { return reinterpret_cast<const uint8_t*>(CFDataGetBytePtr(data.get())); },
> 
> No typecast needed any more: CFDataGetBytePtr returns const uint8_t*.
> 
> > Source/WebCore/platform/SharedBuffer.cpp:235
> > +        [](const GRefPtr<GBytes>& data) { return reinterpret_cast<const uint8_t*>(g_bytes_get_data(data.get(), nullptr)); },
> 
> This should be static_cast, not reinterpret_cast; it’s a cast from a void*.
> 
> > Source/WebCore/platform/SharedBuffer.cpp:238
> > +        [](const RefPtr<GstMappedOwnedBuffer>& data) { return reinterpret_cast<const uint8_t*>(data->data()); },
> 
> No typecast needed any more: GstMappedOwnedBuffer::data returns const
> uint8_t*.
> 
> > Source/WebCore/platform/SharedBuffer.cpp:240
> > +        [](const FileSystem::MappedFileData& data) { return reinterpret_cast<const uint8_t*>(data.data()); }
> 
> This should be static_cast, not reinterpret_cast; it’s a cast from a void*.

Fixing all those.

> 
> > Source/WebCore/platform/SharedBuffer.cpp:357
> > +    char* p = reinterpret_cast<char*>(buffer.data());
> 
> Always a sad moment when we have to add a reinterpret_cast; any way to avoid
> this one?
> 
> > Source/WebCore/platform/SharedBuffer.cpp:370
> > +    buffer.shrink(p - reinterpret_cast<char*>(buffer.data()));
> 
> Always a sad moment when we have to add a reinterpret_cast; any way to avoid
> this one?
> 
> > Source/WebCore/platform/SharedBufferChunkReader.cpp:103
> > +                chunk.append(reinterpret_cast<const uint8_t*>(m_separator.data()), m_separatorIndex);
> 
> Always a sad moment when we have to add a reinterpret_cast; any way to avoid
> this one?

Will check.

> 
> > Source/WebCore/platform/graphics/WOFFFileFormat.cpp:297
> > -        buffer = SharedBuffer::create(WTFMove(convertedFont));
> > +        buffer = SharedBuffer::create(convertedFont.data(), convertedFont.size());
> 
> I think this is now copying instead of adopting the Vector. That’s not as
> good.
> 
> > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:239
> > +    const uint8_t* source = sharedBuffer->data() + position;
> 
> auto
> 
> > Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:89
> > +        size_t offset = reinterpret_cast<const uint8_t*>(position) - buffer.data();
> 
> This can be static_cast instead of reinterpret_cast since it’s from a void*.
> 
> > Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:318
> > +    return strncmp(&m_data->dataAsCharPtr()[imageOffset], "\x89PNG", 4) ? BMP : PNG;
> 
> This should use memcmp instead of strncmp--it was never useful to use
> strncmp here, no reason to handle null characters specially--and then we
> will not need dataAsCharPtr here.
> 
> > Source/WebCore/platform/network/FormData.cpp:417
> > +    auto bytes = flatten();
> > +    return SharedBuffer::create(bytes.data(), bytes.size());
> 
> I think this is now copying instead of adopting the Vector. That’s not as
> good.
> 
> > Source/WebCore/platform/win/SharedBufferWin.cpp:55
> > +        if (ReadFile(fileHandle, (LPVOID)buffer.data(), bytesToRead, &bytesRead, 0) && bytesToRead == bytesRead)
> 
> Why was this new typecast needed?

Probably not needed, will drop.

> 
> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:362
> > -    OffsetBuffer(Vector<char> buffer)
> > +    OffsetBuffer(Vector<uint8_t>&& buffer)
> 
> Nice optimization.
> 
> > Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:364
> >          , m_currentOffset(0)
> 
> We should move this initialization down to where the data member is defined
> below.

Will fix.

> 
> > Source/WebKit/WebProcess/Plugins/Netscape/NetscapePluginStream.cpp:120
> > +    deliverData(reinterpret_cast<const uint8_t*>(resultCString.data()), resultCString.length());
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1172
> > +    m_pluginView->manualLoadDidReceiveData(reinterpret_cast<const uint8_t*>(data), length);
> 
> Always a sad day when we have to add a reinterpret_cast. Any way to avoid it?

Will check. Part of the issue is that I was trying to keep the size of this patch small. Switching to uint8_t propagates through the code base quickly. Right now, all the didReceiveData() functions take a const char* and I was planning to switch them all to const uint8_t* in the follow-up given the size.
Comment 12 Chris Dumez 2021-06-04 14:32:06 PDT
Created attachment 430613 [details]
Patch
Comment 13 Chris Dumez 2021-06-04 14:43:16 PDT
Created attachment 430614 [details]
Patch
Comment 14 Chris Dumez 2021-06-04 14:50:23 PDT
Created attachment 430615 [details]
Patch
Comment 15 Chris Dumez 2021-06-04 15:01:48 PDT
Created attachment 430617 [details]
Patch
Comment 16 Chris Dumez 2021-06-04 15:26:23 PDT
Created attachment 430620 [details]
Patch
Comment 17 Chris Dumez 2021-06-04 15:32:58 PDT
Created attachment 430622 [details]
Patch
Comment 18 Chris Dumez 2021-06-04 15:43:44 PDT
Created attachment 430624 [details]
Patch
Comment 19 Chris Dumez 2021-06-04 16:05:12 PDT
Created attachment 430628 [details]
Patch
Comment 20 Sam Weinig 2021-06-04 16:17:55 PDT
Not to throw a wrench into this, but should we consider std::byte instead? That would be my preference all things being equal.
Comment 21 Sam Weinig 2021-06-04 16:18:51 PDT
(In reply to Sam Weinig from comment #20)
> Not to throw a wrench into this, but should we consider std::byte instead?
> That would be my preference all things being equal.

That said, this is a great improvement, and we can always make that change another time if we want to, so don't block yourself on this question too much.
Comment 22 Chris Dumez 2021-06-04 16:19:13 PDT
(In reply to Sam Weinig from comment #20)
> Not to throw a wrench into this, but should we consider std::byte instead?
> That would be my preference all things being equal.

lol, you're killing me. I saw you switch code from `const char*` to `const uint8_t*` just recently in one of your patches :)
Comment 23 Sam Weinig 2021-06-04 16:32:31 PDT
(In reply to Chris Dumez from comment #22)
> (In reply to Sam Weinig from comment #20)
> > Not to throw a wrench into this, but should we consider std::byte instead?
> > That would be my preference all things being equal.
> 
> lol, you're killing me. I saw you switch code from `const char*` to `const
> uint8_t*` just recently in one of your patches :)

Ha! Sorry! As I said, I think this is a great improvement and makes things way more consistent. std::byte is more controversial, so I didn't want to make that change while making the base64 change. Having this in will make that kind of change much easier, so its all win-win. I just thought you and Darin would be a good audience for the question :).
Comment 24 Chris Dumez 2021-06-04 16:59:04 PDT
(In reply to Sam Weinig from comment #23)
> (In reply to Chris Dumez from comment #22)
> > (In reply to Sam Weinig from comment #20)
> > > Not to throw a wrench into this, but should we consider std::byte instead?
> > > That would be my preference all things being equal.
> > 
> > lol, you're killing me. I saw you switch code from `const char*` to `const
> > uint8_t*` just recently in one of your patches :)
> 
> Ha! Sorry! As I said, I think this is a great improvement and makes things
> way more consistent. std::byte is more controversial, so I didn't want to
> make that change while making the base64 change. Having this in will make
> that kind of change much easier, so its all win-win. I just thought you and
> Darin would be a good audience for the question :).

I hadn't heard about std::byte before honestly. I kinda like the strong typing. However, I have a feeling that using it may cause us to have have to reinterpret_cast more, in cases where we call into library functions or system calls that expect a "const uint8_t*"? There's plenty of usage of `const uint8_t*` / `const unsigned char*` out there but not much `const std::byte*`.

For now, my priority is for us to be consistent. Right now we have some classes that use uint8_t and some others char.
Comment 25 EWS 2021-06-04 18:16:52 PDT
Committed r278516 (238515@main): <https://commits.webkit.org/238515@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430628 [details].
Comment 26 Radar WebKit Bug Importer 2021-06-04 18:17:25 PDT
<rdar://problem/78894758>
Comment 27 Darin Adler 2021-06-04 21:08:51 PDT
Wow, getting to std::byte at *some* point seems pretty neat. I want to learn more about it.