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


Can we also introduce a corresponding `.cpp` for the implementation?


3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139914>

    The pattern in Mesos for output streamers is such that we define the 
behavior in `operator<<` and `friend` it from the class.



3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139916>

    (1) `s/struct//`
    (2) `= {};`



3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139922>

    (1) If we want to zero-initialize the array, we should do `= {};`
    (2) Can we choose a power of 2 for the size?
    (3) In `RFC3339`, we name this `date`. Let's call them both `buffer` or 
both `date`.
    (4) Let's move this down past `HTTP_DATE`, `WEEK_DAYS` and `MONTHS`, just 
above the `snprintf` call.



3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139933>

    Can we just inline this? We inline the format string in `RFC3339`



3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139923>

    We should indent 4 spaces from the beginning of the function call:
    
    ```
         if (snprintf(
                 buffer,
                 sizeof(buffer),
                 HTTP_DATE,
                 WEEK_DAYS[timeInfo.tm_wday],
                 timeInfo.tm_mday,
                 MONTHS[timeInfo.tm_mon],
                 timeInfo.tm_year + 1900,
                 timeInfo.tm_hour,
                 timeInfo.tm_min,
                 timeInfo.tm_sec) < 0) {
    ```



3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139927>

    No need to construct a `std::string` here.



3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139920>

    `s/tm_/timeInfo/`
    `= {};`



3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139921>

    (1) If we want to zero-initialize the array, we should do `= {};`
    (2) Can we choose a power of 2 for the size?



3rdparty/libprocess/include/process/time.hpp
<https://reviews.apache.org/r/34703/#comment139932>

    Should be 2-space indented.



3rdparty/libprocess/src/tests/time_tests.cpp
<https://reviews.apache.org/r/34703/#comment139931>

    We don't need to use `stringstream` here to use the `operator<<` since 
`stringify` uses it.
    
    We can simplify to:
    
    ```
    EXPECT_EQ("Thu, 02 Mar 1989 00:00:00 UTC",
              stringify(RFC1123(Time::epoch() + Weeks(1000))));
    ```


- Michael Park


On June 8, 2015, 3:14 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34703/
> -----------------------------------------------------------
> 
> (Updated June 8, 2015, 3:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and 
> Till Toenshoff.
> 
> 
> Bugs: mesos-708
>     https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds some manipulator classes which allows formatting Time objects to 
> ostreams.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/time.hpp 
> c5ab2a3cfa83590eb6612152ae365dd67f51cea9 
>   3rdparty/libprocess/src/tests/time_tests.cpp 
> be314182c65c05d439b81aa5248a71d93f6f0a0b 
> 
> Diff: https://reviews.apache.org/r/34703/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to