Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review91100 --- Ship it! Will fix the below issue while comitting. 3rdparty/libprocess/src/time.cpp (line 1) https://reviews.apache.org/r/34703/#comment144366 This misses the “Apache License Version 2.0” header - see our style-guide on File Headers. ``` /** * Licensed under the Apache License, Version 2.0 (the License); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an AS IS BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License */ ``` - Till Toenshoff On June 17, 2015, 3:42 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated June 17, 2015, 3:42 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/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b 3rdparty/libprocess/src/time.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated June 17, 2015, 5:42 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Changes --- Addresses Bernd's comments. 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 (updated) - 3rdparty/libprocess/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b 3rdparty/libprocess/src/time.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34703: Added stream manipulators for the Time object.
On June 16, 2015, 10:29 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/src/time.cpp, line 41 https://reviews.apache.org/r/34703/diff/5/?file=984729#file984729line41 Why can't we use strfmt_l with a locale argument that matches the output we want? `strfmt_l` is available only on OS X. On June 16, 2015, 10:29 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/src/time.cpp, line 42 https://reviews.apache.org/r/34703/diff/5/?file=984729#file984729line42 s/the RFC 1123/RFC1123 Here is not talking about the struct `RFC11123` but to the actual [RFC 1123](https://www.ietf.org/rfc/rfc1123.txt) - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review88038 --- On June 16, 2015, 2:24 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated June 16, 2015, 2:24 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/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b 3rdparty/libprocess/src/time.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated June 16, 2015, 2:24 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Changes --- Bernd's review. 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 (updated) - 3rdparty/libprocess/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b 3rdparty/libprocess/src/time.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review88038 --- 3rdparty/libprocess/src/tests/time_tests.cpp (line 42) https://reviews.apache.org/r/34703/#comment140444 I suspect we need more test coverage than this. What are the boundary cases? Are they all checked? 3rdparty/libprocess/src/tests/time_tests.cpp (line 60) https://reviews.apache.org/r/34703/#comment140445 I suspect we need more test coverage than this. What are the boundary cases? Are they all checked? 3rdparty/libprocess/src/time.cpp (line 16) https://reviews.apache.org/r/34703/#comment140441 s/the 'time'/a 'time_t' value 3rdparty/libprocess/src/time.cpp (line 41) https://reviews.apache.org/r/34703/#comment140443 Why can't we use strfmt_l with a locale argument that matches the output we want? 3rdparty/libprocess/src/time.cpp (line 42) https://reviews.apache.org/r/34703/#comment140442 s/the RFC 1123/RFC1123 3rdparty/libprocess/src/time.cpp (line 85) https://reviews.apache.org/r/34703/#comment140438 When I first read this, I thought that nsecs stands for number_of_seconds. Only by reading the comment above did I find out that you mean nano seconds. Please use unambiguous, non-abbreviated identifiers - even if the 3rd party library functions in the vicinity have sloppy naming. By using nanoSeconds for the variable name, part of the comment will become redundant! 3rdparty/libprocess/src/time.cpp (line 86) https://reviews.apache.org/r/34703/#comment140440 If you leave this line blank, it seems that the Append part of the comment above does not apply to what follows as of line 87. Alternatively add a blank line between 84 and 85. - Bernd Mathiske On June 15, 2015, 8:26 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated June 15, 2015, 8:26 a.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/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b 3rdparty/libprocess/src/time.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated June 15, 2015, 5:26 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Changes --- mpark's comments. Changes serialization from UTC to GMT to get around a bug in osx (see man strptime) 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 (updated) - 3rdparty/libprocess/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 3rdparty/libprocess/include/process/time.hpp c5ab2a3cfa83590eb6612152ae365dd67f51cea9 3rdparty/libprocess/src/tests/time_tests.cpp be314182c65c05d439b81aa5248a71d93f6f0a0b 3rdparty/libprocess/src/time.cpp PRE-CREATION Diff: https://reviews.apache.org/r/34703/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review87550 --- 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139934 `s/constexpr/static/` 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139936 Formatting: ```static const char* WEEK_DAYS[] = { Sun, Mon, Tue, Wed, Thu, Fri, Sat }; ``` 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139935 `s/constexpr/static/` 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139937 Formatting: ``` static const char* MONTHS[] = { Jan, Feb, Mar, Apr, May, Jun, Jul, Aug, Sep, Oct, Nov, Dec }; ``` - 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
Re: Review Request 34703: Added stream manipulators for the Time object.
On June 11, 2015, 11:10 a.m., Michael Park wrote: 3rdparty/libprocess/include/process/time.hpp, lines 111-114 https://reviews.apache.org/r/34703/diff/4/?file=980765#file980765line111 Formatting: ```static const char* WEEK_DAYS[] = { Sun, Mon, Tue, Wed, Thu, Fri, Sat }; ``` Sorry, not sure what happened there. I meant ```cpp static const char* WEEK_DAYS[] = { Sun, Mon, Tue, Wed, Thu, Fri, Sat }; ``` - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review87550 --- 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
Re: Review Request 34703: Added stream manipulators for the Time object.
--- 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
Re: Review Request 34703: Added stream manipulators for the Time object.
On June 8, 2015, 2:59 p.m., Till Toenshoff wrote: 3rdparty/libprocess/include/process/time.hpp, line 150 https://reviews.apache.org/r/34703/diff/3/?file=974298#file974298line150 Lets think about a good replacement for that class name to make it intuitive - first thing that came to mind is ```UnixTimestamp```. Lets also add an example right here in this comment. Apparently BenH is OK with leaving the RFC as part of the name. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review87013 --- On June 8, 2015, 5: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, 5: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
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review87028 --- 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139287 Move the opening brace to the first line please (like you did below). 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139286 Lets make this a copy instead to prevent weird bugs introduced by taking references to temporals. 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139288 `const Time time;` - Till Toenshoff On May 29, 2015, 12:59 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 29, 2015, 12:59 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
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review87013 --- Looks pretty good. I just am unsure about the class naming - the rest is just nits. 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139262 Can we give this a different, more intuitive name please? I would suggest `HTTPDate` instead. Can we please also add an example so people looking for a particular format will quickly find it? 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139263 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139264 `snprintf` does not seem to set `errno`, hence `LOG` should be used instead of `PLOG`. 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139265 2 chars indent please. 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139275 Lets think about a good replacement for that class name to make it intuitive - first thing that came to mind is ```UnixTimestamp```. Lets also add an example right here in this comment. 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment139276 2 chars intention please. - Till Toenshoff On May 29, 2015, 12:59 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 29, 2015, 12:59 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
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 29, 2015, 2:59 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Changes --- Added constexpr 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 (updated) - 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
Re: Review Request 34703: Added stream manipulators for the Time object.
On May 28, 2015, 7:49 a.m., Nikita Vetoshkin wrote: 3rdparty/libprocess/include/process/time.hpp, line 109 https://reviews.apache.org/r/34703/diff/2/?file=972867#file972867line109 Can I add one more nitpick? If we make `WEEK_DAYS` and `MONTHS` `static` then will put only array (which is initialised staticly and save in binary) pointer on stack instead of recreating array every time. Here's my code: ``` const char *WEEK_DAYS[] = {Sun, Mon, Tue, Wed, Thu, Fri, Sat}; std::cout WEEK_DAYS[i] std::endl; ``` Here's a relevant snippet of g++ assempler output: ``` movq$.LC0, (%rsp) movq$.LC1, 8(%rsp) movq$.LC2, 16(%rsp) movq$.LC3, 24(%rsp) movq$.LC4, 32(%rsp) movq$.LC5, 40(%rsp) movq$.LC6, 48(%rsp) movq(%rsp,%rdi,8), %rsi movl$_ZSt4cout, %edi ``` If we make it `const`, then it starts looking like this: ``` movq_ZZ4mainE9WEEK_DAYS(,%rdi,8), %rsi movl$_ZSt4cout, %edi ``` Alexander Rojas wrote: The project itself is not so fond of using static function objects (I tried). The idea is to leave it as const and later on marked as constexpr. I added constexpr. I haven't check the assembler output, but ir should make the trick. However, if it doesn't, I'm dropping this issue since static function variables are not used within mesos. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review85514 --- On May 29, 2015, 2:59 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 29, 2015, 2:59 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
Re: Review Request 34703: Added stream manipulators for the Time object.
On May 28, 2015, 5:49 a.m., Nikita Vetoshkin wrote: 3rdparty/libprocess/include/process/time.hpp, line 109 https://reviews.apache.org/r/34703/diff/2/?file=972867#file972867line109 Can I add one more nitpick? If we make `WEEK_DAYS` and `MONTHS` `static` then will put only array (which is initialised staticly and save in binary) pointer on stack instead of recreating array every time. Here's my code: ``` const char *WEEK_DAYS[] = {Sun, Mon, Tue, Wed, Thu, Fri, Sat}; std::cout WEEK_DAYS[i] std::endl; ``` Here's a relevant snippet of g++ assempler output: ``` movq$.LC0, (%rsp) movq$.LC1, 8(%rsp) movq$.LC2, 16(%rsp) movq$.LC3, 24(%rsp) movq$.LC4, 32(%rsp) movq$.LC5, 40(%rsp) movq$.LC6, 48(%rsp) movq(%rsp,%rdi,8), %rsi movl$_ZSt4cout, %edi ``` If we make it `const`, then it starts looking like this: ``` movq_ZZ4mainE9WEEK_DAYS(,%rdi,8), %rsi movl$_ZSt4cout, %edi ``` Alexander Rojas wrote: The project itself is not so fond of using static function objects (I tried). The idea is to leave it as const and later on marked as constexpr. Alexander Rojas wrote: I added constexpr. I haven't check the assembler output, but ir should make the trick. However, if it doesn't, I'm dropping this issue since static function variables are not used within mesos. No, it does not. Array is constructed each time :) - Nikita --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review85514 --- On May 29, 2015, 12:59 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 29, 2015, 12:59 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
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review85380 --- 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment136923 This will result in 7 (for each day of week) + 1 (for vector itself) dynamic allocations on each call. Can/should we avoid them using C style structures? Same applies to MONTHS variable. - Nikita Vetoshkin On May 27, 2015, 2:48 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 27, 2015, 2:48 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
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 27, 2015, 4:48 p.m.) Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and Till Toenshoff. Changes --- It really doesn't have a dependency. 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
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review85364 --- Patch looks great! Reviews applied: [34392, 34703] All tests passed. - Mesos ReviewBot On May 27, 2015, 1:33 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 27, 2015, 1:33 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
Re: Review Request 34703: Added stream manipulators for the Time object.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review85514 --- 3rdparty/libprocess/include/process/time.hpp https://reviews.apache.org/r/34703/#comment137084 Can I add one more nitpick? If we make `WEEK_DAYS` and `MONTHS` `static` then will put only array (which is initialised staticly and save in binary) pointer on stack instead of recreating array every time. Here's my code: ``` const char *WEEK_DAYS[] = {Sun, Mon, Tue, Wed, Thu, Fri, Sat}; std::cout WEEK_DAYS[i] std::endl; ``` Here's a relevant snippet of g++ assempler output: ``` movq$.LC0, (%rsp) movq$.LC1, 8(%rsp) movq$.LC2, 16(%rsp) movq$.LC3, 24(%rsp) movq$.LC4, 32(%rsp) movq$.LC5, 40(%rsp) movq$.LC6, 48(%rsp) movq(%rsp,%rdi,8), %rsi movl$_ZSt4cout, %edi ``` If we make it `const`, then it starts looking like this: ``` movq_ZZ4mainE9WEEK_DAYS(,%rdi,8), %rsi movl$_ZSt4cout, %edi ``` - Nikita Vetoshkin On May 27, 2015, 3:46 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated May 27, 2015, 3:46 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