----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34068/#review84370 -----------------------------------------------------------
Thanks for the test! I left some comments that are relevant to https://reviews.apache.org/r/33793/ as well, so be sure to update that review too. 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135585> Let's call this something that tells us what it does, how about: 'CaseInsensitiveHash'? 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135582> Style guide for this is: ``` size_t operator () (...) const { } ``` Note the whitespace and that braces should be on the next line for functions. 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135586> Don't think we've been using std::size_t instead of just size_t in the rest of the code. 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135589> You need <ctype.h> for tolower. Also, we haven't been using the C++ include version <cctype> with the std:: prefix. So you can remove the std:: qualifier here. Also, do you need an include for hash_combine? ``` #include <boost/functional/hash.hpp> ``` 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135581> Let's call this 'CaseInsensitiveEqual' to make it easier for someone to understand when they're reading this code :) 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135583> Ditto here for whitespace and brace on the next line. 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135588> This looks pretty tricky to understand, also note that you're looping the shorter of the two strings even if they are not the same length. How about the following? ``` { if (left.size() != right.size()) { return false; } for (size_t i = 0; i < left.size(); ++i) { if (std::tolower(c } return true; } ``` 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135591> Please avoid printing unnecessary output in the test :) 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135592> Ditto here. 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp <https://reviews.apache.org/r/34068/#comment135594> Most of these look like they can be EXPECTs instead of ASSERTs, the latter you want only when the test cannot proceed if the assertion fails. But in most of the cases you here, it's ok to continue checking things. Can you re-organize this a bit, it's hard to follow that you're testing the things we care about. Also, how about just sticking with 'put' and 'get', rather than also using '[]' and 'contains' here? - Ben Mahler On May 12, 2015, 12:56 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34068/ > ----------------------------------------------------------- > > (Updated May 12, 2015, 12:56 a.m.) > > > Review request for mesos, Alexander Rojas and Ben Mahler. > > > Repository: mesos > > > Description > ------- > > The test case of extend hashmap to support custom equality and hash > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/tests/hashmap_tests.cpp > e8a932e5474bf2ba1a93a945ff9bc61fb5146c02 > > Diff: https://reviews.apache.org/r/34068/diff/ > > > Testing > ------- > > make check > > > Thanks, > > haosdent huang > >
