Bug 173253 - [GCrypt] Use utility functions in CryptoKeyEC, CryptoKeyRSA
Summary: [GCrypt] Use utility functions in CryptoKeyEC, CryptoKeyRSA
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: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 133122
  Show dependency treegraph
 
Reported: 2017-06-12 03:09 PDT by Zan Dobersek
Modified: 2017-06-12 07:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch (13.73 KB, patch)
2017-06-12 03:15 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-06-12 03:09:55 PDT
[GCrypt] Use utility functions in CryptoKeyEC, CryptoKeyRSA
Comment 1 Zan Dobersek 2017-06-12 03:15:09 PDT
Created attachment 312647 [details]
Patch
Comment 2 Michael Catanzaro 2017-06-12 05:56:17 PDT
Comment on attachment 312647 [details]
Patch

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

Nice!

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:227
> +    return WTFMove(q.value());

Why do you use WTFMove() here? Doesn't this prevent return value optimization?

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:70
> +    return WTFMove(data.value());

Ditto.
Comment 3 Zan Dobersek 2017-06-12 07:55:28 PDT
Comment on attachment 312647 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:227
>> +    return WTFMove(q.value());
> 
> Why do you use WTFMove() here? Doesn't this prevent return value optimization?

RVO doesn't help here because q is a std::optional<> object. `q.value()` exposes the embedded Vector<> object, but returning just that would result in copying the object. So the embedded value has to be casted to an rvalue reference via WTFMove() to enforce the move constructor to be generated.

So the Vector<> object inside std::optional<> is moved into the return value, and after that std::optional<> deletes the emptied-out Vector<> it still holds.
Comment 4 Zan Dobersek 2017-06-12 07:56:38 PDT
Comment on attachment 312647 [details]
Patch

Clearing flags on attachment: 312647

Committed r218100: <http://trac.webkit.org/changeset/218100>
Comment 5 Zan Dobersek 2017-06-12 07:56:42 PDT
All reviewed patches have been landed.  Closing bug.