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