-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38158/#review97983
-----------------------------------------------------------

Ship it!


Looks really good to me, thanks Joerg.


src/common/values.cpp (line 125)
<https://reviews.apache.org/r/38158/#comment154729>

    I like this explicit way of calculating the size via a scoped variable.



src/common/values.cpp (lines 149 - 150)
<https://reviews.apache.org/r/38158/#comment154730>

    :D



src/common/values.cpp (line 162)
<https://reviews.apache.org/r/38158/#comment154731>

    Why this blank line?



src/common/values.cpp (line 179)
<https://reviews.apache.org/r/38158/#comment154732>

    US english would be s/neighbouring/neighboring/



src/common/values.cpp (lines 210 - 211)
<https://reviews.apache.org/r/38158/#comment154733>

    I think this would look less "noisy":
    ```
    void addRange(
        Value::Ranges* ranges, uint64_t begin, uint64_t end, int insertPosition)
    ```



src/tests/values_tests.cpp (line 37)
<https://reviews.apache.org/r/38158/#comment154734>

    Fits a single line:
    ```
      extern void coalesce(Value::Ranges* ranges, const Value::Range& range);
    ```



src/tests/values_tests.cpp (line 39)
<https://reviews.apache.org/r/38158/#comment154735>

    Fits a single line:
    ```
      extern void removeRanges(Value::Ranges* ranges, std::set<int>* deleteSet);
    ```



src/tests/values_tests.cpp (lines 41 - 42)
<https://reviews.apache.org/r/38158/#comment154736>

    How about this instead?
    ```
      extern void addRange(
          Value::Ranges* ranges, uint64_t begin, uint64_t end, int 
insertPosition);
    ```



src/tests/values_tests.cpp (line 48)
<https://reviews.apache.org/r/38158/#comment154747>

    Not yours but IIUC then this is a blank line too many.



src/tests/values_tests.cpp (line 218)
<https://reviews.apache.org/r/38158/#comment154746>

    Insert blank please.



src/tests/values_tests.cpp (line 253)
<https://reviews.apache.org/r/38158/#comment154737>

    s/Overlap/overlap/



src/tests/values_tests.cpp (line 305)
<https://reviews.apache.org/r/38158/#comment154738>

    s/overlap/overlaps/ ?



src/tests/values_tests.cpp (line 319)
<https://reviews.apache.org/r/38158/#comment154739>

    Not sure if we should aim for commonwealth or us english on the 
neighbour/neighbor? :)



src/tests/values_tests.cpp (line 326)
<https://reviews.apache.org/r/38158/#comment154740>

    s/N/n/



src/tests/values_tests.cpp (line 333)
<https://reviews.apache.org/r/38158/#comment154741>

    s/collescing/coalescing/



src/tests/values_tests.cpp (line 340)
<https://reviews.apache.org/r/38158/#comment154742>

    see above



src/tests/values_tests.cpp (line 376)
<https://reviews.apache.org/r/38158/#comment154743>

    Hah, now you chose the US-english "neighbor" :D



src/tests/values_tests.cpp (line 390)
<https://reviews.apache.org/r/38158/#comment154744>

    Quick descriptive comment please.



src/tests/values_tests.cpp (line 430)
<https://reviews.apache.org/r/38158/#comment154745>

    Quick descriptive comment please.


- Till Toenshoff


On Sept. 9, 2015, 8:48 p.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2015, 8:48 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
>     https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -----
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Reply via email to