Bug 225971

Summary: Drop "get" prefix from SQLiteStatement member functions as well as out-parameters
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, beidson, benjamin, berto, cgarcia, cmarcelo, darin, ews-watchlist, galpeter, ggaren, gustavo, japhet, jsbell, sam, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

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>