[webkit-reviews] review granted: [Bug 170238] [GCrypt] Add a Handle<> class to help with GCrypt object lifetime control : [Attachment 305727] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 29 08:35:13 PDT 2017

Michael Catanzaro <mcatanzaro at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 170238: [GCrypt] Add a Handle<> class to help with GCrypt object lifetime

Attachment 305727: Patch


--- Comment #2 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 305727
  --> https://bugs.webkit.org/attachment.cgi?id=305727

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


> Source/WebCore/PAL/ChangeLog:20
> +	   The address of the managed handle can be retrieved through the
> +	   operator. An implicit conversion operator is also added. This allows
> +	   frictionless use of GCrypt::Handle<> objects with existing libgcrypt

It's also not how resources handles work in WebKit or the standard library. We
add a .get() method to get a pointer to the underlying object. So does the
standard library. Why not follow this convention?

> Source/WebCore/PAL/ChangeLog:24
> +	   the managed handle. The raw handle value is also retrieveable
> +	   the handle() method.

Do you really prefer .handle() to .get()?

> Source/WebCore/PAL/ChangeLog:41
> +	   std::unique_ptr<> could be used, but it could also be mis-used. I
> +	   a mini-class with an interface that's specific to libgcrypt API
> +	   interactions to be preferrable to a std::unique_ptr<> with a custom
> +	   deleter.

Yes, although I would have expected GCrypt::Handle to be implemented in terms
of std::unique_ptr. Any particular reason you decided not to do that?

> Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:31
> +namespace GCrypt {

This is the top-level namespace? Surely it should be put under PAL as well? I
know it's going to be annoying to type PAL::GCrypt everywhere we use it, but
that's a generic problem of PAL and not something to be solved here.

> Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:66
> +    T release()

Why does it return a T and not a T*? I guess the handle is intended to be
copied by value mostly?

> Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:75
> +    T handle() const { return m_handle; }

Same question here. I suppose it does make a lot of sense to call this
.handle() instead of .get() if it's going to be returning a value and not a

> Source/WebCore/PAL/pal/crypto/gcrypt/Handle.h:76
> +    operator T() const { return m_handle; }

The implicit conversion makes me a little nervous, since it does not match our
pattern for smart pointers, but I suppose this is really a different type of
object, so I think it's OK. Certainly it would be more convenient to use.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:38
> +#include <PAL/pal/crypto/gcrypt/Handle.h>

No, this isn't OK. We don't use absolute paths to include directories in
WebKit. I'd actually prefer if we did, but we're not going to change that
today. You'll need to add PAL/pal/crypto/gcrypt to the include path.

More information about the webkit-reviews mailing list