This is an automatically generated e-mail. To reply, visit:

3rdparty/libprocess/include/process/digest.hpp (line 39)

    Kill the line here.

3rdparty/libprocess/include/process/digest.hpp (line 71)

    I imagine `fn` is expensive, after all, it's  cryptography. :)
    Serially doing read and compute seems to be suboptimal but I simply 
parallelize read and compute can cause OOM when read is faster than compute. Of 
couse this is a classic producer and consumer problem and we can use a queue. 
We probably don't want to make things too complicated here in a single review 
but again, this is the issue of reimplemting a generic and widely used utility: 
we don't get to leverage state of the art for free...
    Just a comment.

3rdparty/libprocess/include/process/digest.hpp (line 84)

    No space before and after the conditional guard. Here and elsewhere.

3rdparty/libprocess/include/process/digest.hpp (lines 85 - 120)

    These templates (here and below) have different levels of specializations, 
traits, typedefs in various places, which is hard to understand.
    Do you think the following is simpler?
    For each digest type, there is one **low-level** implementation, i.e., 
Init, Update and Final are called separatedly but they expose one common 
interface (without digest-type specific) arguments.
    class DigestImpl
      void init() == 0; 
      int update(const void*, size_t) == 0; 
      int final(uint8_t*) == 0;
      static Try<DigestImpl*> create(...);
    You have a low implementation for each type that inherits this interface 
and encapsulates its specific context variables in its member variables. The 
low-level implementation should be simply calling openssl APIs so it should be 
short and has no more redundant code among different implementations than 
specializations in different places and this consolidates handling of specific 
digest-types in one single place.
    The high-level DigestUtil implementation or simply, the static generic 
method implementation can just create a low-level impl class and you can put 
common logic there.
    What do you think?

3rdparty/libprocess/include/process/digest.hpp (line 125)

    No indentation in a new line in this case, here and elsewhere.

3rdparty/libprocess/include/process/digest.hpp (lines 159 - 160)

    About templatization. I previously commented that the implementation should 
be put in the CPP file.
    You use templates but your interafce is not templatized and used only by 
this single component. Can't all the templates just be in the same source file 
as the caller so that the header only has the API declarations?
    This of course becomes a moot point is we don't use template as I was 
suggesting above.

- Jiang Yan Xu

On Oct. 12, 2015, 2:14 p.m., Jojy Varghese wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38747/
> -----------------------------------------------------------
> (Updated Oct. 12, 2015, 2:14 p.m.)
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> Repository: mesos
> Description
> -------
> Added SHA256, SHA512 implementation.
> Diffs
> -----
>   3rdparty/libprocess/Makefile.am eb7393904988afc0bc5e9f7dadd545e0d6c04e5f 
>   3rdparty/libprocess/include/Makefile.am 
> fcc62e99b92b9d2e7ab344e561a06dd6de1fef7e 
>   3rdparty/libprocess/include/process/digest.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> a14b5b8fe22d3e75bef3c716ae7865ddaf132927 
>   3rdparty/libprocess/src/tests/digest_tests.cpp PRE-CREATION 
> Diff: https://reviews.apache.org/r/38747/diff/
> Testing
> -------
> make check.
> Thanks,
> Jojy Varghese

Reply via email to