mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 34068: The test case of extend hashmap to support custom equality and hash
Date Tue, 19 May 2015 20:05:57 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message