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
