Bug 173253

Summary: [GCrypt] Use utility functions in CryptoKeyEC, CryptoKeyRSA
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: jiewen_tan, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 133122    
Attachments:
Description Flags
Patch none

Zan Dobersek
Reported 2017-06-12 03:09:55 PDT
[GCrypt] Use utility functions in CryptoKeyEC, CryptoKeyRSA
Attachments
Patch (13.73 KB, patch)
2017-06-12 03:15 PDT, Zan Dobersek
no flags
Zan Dobersek
Comment 1 2017-06-12 03:15:09 PDT
Michael Catanzaro
Comment 2 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.
Zan Dobersek
Comment 3 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.
Zan Dobersek
Comment 4 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>
Zan Dobersek
Comment 5 2017-06-12 07:56:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.