Bug 225971 - Drop "get" prefix from SQLiteStatement member functions as well as out-parameters
Summary: Drop "get" prefix from SQLiteStatement member functions as well as out-parame...
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-05-19 09:54 PDT by Chris Dumez
Modified: 2021-05-19 14:01 PDT (History)
17 users (show)

See Also:


Attachments
Patch (107.80 KB, patch)
2021-05-19 09:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (110.51 KB, patch)
2021-05-19 12:03 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-05-19 09:54:11 PDT
Drop "get" prefix from SQLiteStatement member functions as well as out-parameters.
Comment 1 Chris Dumez 2021-05-19 09:59:49 PDT
Created attachment 429056 [details]
Patch
Comment 2 EWS Watchlist 2021-05-19 10:00:38 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Darin Adler 2021-05-19 11:22:14 PDT
Comment on attachment 429056 [details]
Patch

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

> Source/WebCore/platform/sql/SQLiteStatement.cpp:287
> +    Vector<uint8_t> result;

I think we should declare this later and just return { } in earlier return statements.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:304
> +    result.resize(static_cast<size_t>(blobSize));
> +    memcpy(result.data(), blob, static_cast<size_t>(blobSize));
> +    return result;

To construct a vector with a copy of the data, we shouldn’t use resize. I wish there was a suitable Vector constructor so you could write this:

    return { blob, static_cast<size_t>(blobSize) };

But instead we need to write this:

    Vector<uint8_t> result;
    result.reserveInitialCapacity(blobSize);
    result.append(blob, blobSize);
    return result;

Also maybe we should consider having this return FixedVector in the future?
Comment 4 Chris Dumez 2021-05-19 12:03:13 PDT
Created attachment 429083 [details]
Patch
Comment 5 Chris Dumez 2021-05-19 12:03:37 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 429056 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429056&action=review
> 
> > Source/WebCore/platform/sql/SQLiteStatement.cpp:287
> > +    Vector<uint8_t> result;
> 
> I think we should declare this later and just return { } in earlier return
> statements.

Done.

> > Source/WebCore/platform/sql/SQLiteStatement.cpp:304
> > +    result.resize(static_cast<size_t>(blobSize));
> > +    memcpy(result.data(), blob, static_cast<size_t>(blobSize));
> > +    return result;
> 
> To construct a vector with a copy of the data, we shouldn’t use resize. I
> wish there was a suitable Vector constructor so you could write this:
> 
>     return { blob, static_cast<size_t>(blobSize) };

I added this new Vector constructor and an API test for it.
Comment 6 EWS 2021-05-19 14:00:48 PDT
Committed r277750 (237920@main): <https://commits.webkit.org/237920@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429083 [details].
Comment 7 Radar WebKit Bug Importer 2021-05-19 14:01:23 PDT
<rdar://problem/78223453>