----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38747/#review102302 -----------------------------------------------------------
3rdparty/libprocess/include/process/digest.hpp (line 39) <https://reviews.apache.org/r/38747/#comment159996> Kill the line here. 3rdparty/libprocess/include/process/digest.hpp (line 71) <https://reviews.apache.org/r/38747/#comment159907> 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) <https://reviews.apache.org/r/38747/#comment159902> No space before and after the conditional guard. Here and elsewhere. 3rdparty/libprocess/include/process/digest.hpp (lines 85 - 120) <https://reviews.apache.org/r/38747/#comment159980> 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 { public: 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) <https://reviews.apache.org/r/38747/#comment159908> No indentation in a new line in this case, here and elsewhere. 3rdparty/libprocess/include/process/digest.hpp (lines 159 - 160) <https://reviews.apache.org/r/38747/#comment159973> 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 > >
