Bug 227606 - Use more Span
Summary: Use more Span
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-01 18:21 PDT by Alex Christensen
Modified: 2021-07-07 10:08 PDT (History)
11 users (show)

See Also:


Attachments
Patch (35.50 KB, patch)
2021-07-01 18:22 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (35.86 KB, patch)
2021-07-01 19:33 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (36.27 KB, patch)
2021-07-02 17:37 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (37.76 KB, patch)
2021-07-02 22:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (38.33 KB, patch)
2021-07-06 09:19 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (37.81 KB, patch)
2021-07-06 22:05 PDT, Alex Christensen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-07-01 18:21:55 PDT
Use more Span
Comment 1 Alex Christensen 2021-07-01 18:22:54 PDT
Created attachment 432762 [details]
Patch
Comment 2 Alex Christensen 2021-07-01 19:33:35 PDT
Created attachment 432763 [details]
Patch
Comment 3 Alex Christensen 2021-07-02 17:37:59 PDT
Created attachment 432835 [details]
Patch
Comment 4 Alex Christensen 2021-07-02 22:31:35 PDT
Created attachment 432842 [details]
Patch
Comment 5 Alex Christensen 2021-07-06 09:19:50 PDT
Created attachment 432940 [details]
Patch
Comment 6 Darin Adler 2021-07-06 17:12:52 PDT
Comment on attachment 432940 [details]
Patch

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

> Source/WTF/wtf/persistence/PersistentDecoder.h:99
> +    Span<const uint8_t>::iterator m_iterator;

We should just write const uint8_t* for this type.

I don’t think this is an "iterator", it’s just a pointer. If you asked "what’s the iterator type" then, yes, you get the pointer type, but we should not try to be abstract.

I think we should keep the names m_buffer and m_bufferPosition, rather than renaming to m_span and m_iterator.
Comment 7 Alex Christensen 2021-07-06 22:05:16 PDT
Created attachment 433010 [details]
Patch
Comment 8 Darin Adler 2021-07-07 10:02:55 PDT
Comment on attachment 433010 [details]
Patch

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

> Source/WTF/wtf/persistence/PersistentDecoder.cpp:73
> +    // FIXME: check for underflow.
> +    if (m_bufferPosition - size >= m_buffer.begin()) {

Since we know that m_bufferPosition is >= m_buffer.begin(), we can sidestep the possibility of overflow without adding an additional check by writing this:

    if (size <= m_bufferPosition - m_buffer.begin()) {

> Source/WebKit/NetworkProcess/cache/NetworkCacheData.h:77
>      const uint8_t* data() const;
>      size_t size() const { return m_size; }

Maybe drop these eventually?
Comment 9 Alex Christensen 2021-07-07 10:08:44 PDT
Committed to r279645

(In reply to Darin Adler from comment #8)
> Comment on attachment 433010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433010&action=review
> 
> > Source/WTF/wtf/persistence/PersistentDecoder.cpp:73
> > +    // FIXME: check for underflow.
> > +    if (m_bufferPosition - size >= m_buffer.begin()) {
> 
> Since we know that m_bufferPosition is >= m_buffer.begin(), we can sidestep
> the possibility of overflow without adding an additional check by writing
> this:
> 
>     if (size <= m_bufferPosition - m_buffer.begin()) {
Great!  It also needs a static_cast<size_t> because the right side is now signed.
> 
> > Source/WebKit/NetworkProcess/cache/NetworkCacheData.h:77
> >      const uint8_t* data() const;
> >      size_t size() const { return m_size; }
> 
> Maybe drop these eventually?
Hopefully!