[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]

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
control
https://bugs.webkit.org/show_bug.cgi?id=170238

Attachment 305727: Patch

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




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

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

Nice!

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

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
through
> +	   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
find
> +	   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
pointer.

> 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.