You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Joris Van Remoortere <jo...@gmail.com> on 2014/12/19 01:20:17 UTC
Review Request 29226: Add construction and conversion from / to
timeval for Duration.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29226/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Repository: mesos-git
Description
-------
See Summary.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp f3447055eb5a0f6abf55211f337e062ed2cc607e
3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 4269d3c8964dce920beb8a40ef72caf075aa0a66
Diff: https://reviews.apache.org/r/29226/diff/
Testing
-------
make check.
Thanks,
Joris Van Remoortere
Re: Review Request 29226: Add construction and conversion from / to
timeval for Duration.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Dec. 19, 2014, 7:11 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp, line 93
> > <https://reviews.apache.org/r/29226/diff/1/?file=796956#file796956line93>
> >
> > tv_sec and tv_usec are the right types to use, just in case they're not long int on some platforms.
Where do you find these types?
I'm only familiar with them being long int (e.g. http://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html)
I can re-write it to be type agnostic, but then we lose readability:
```
struct timeval tval;
tval.tv_sec = secs();
tval.tv_usec = us() - (tval.tv_sec * MILLISECONDS);
return tval;
```
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29226/#review65654
-----------------------------------------------------------
On Dec. 19, 2014, 12:20 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29226/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2014, 12:20 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See Summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp f3447055eb5a0f6abf55211f337e062ed2cc607e
> 3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 4269d3c8964dce920beb8a40ef72caf075aa0a66
>
> Diff: https://reviews.apache.org/r/29226/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 29226: Add construction and conversion from / to
timeval for Duration.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Dec. 19, 2014, 11:11 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp, line 93
> > <https://reviews.apache.org/r/29226/diff/1/?file=796956#file796956line93>
> >
> > tv_sec and tv_usec are the right types to use, just in case they're not long int on some platforms.
>
> Joris Van Remoortere wrote:
> Where do you find these types?
> I'm only familiar with them being long int (e.g. http://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html)
> I can re-write it to be type agnostic, but then we lose readability:
> ```
> struct timeval tval;
> tval.tv_sec = secs();
> tval.tv_usec = us() - (tval.tv_sec * MILLISECONDS);
> return tval;
> ```
>
> Dominic Hamon wrote:
> i think that's as readable. maybe even:
>
> struct timeval tval;
> tval.tv_sec = secs();
> tval.tv_usec = us() - (secs() * MILLISECONDS);
> return tval;
>
> given that duplicate calls to secs() are cheap and will almost certainly be optimized out.
>
> Joris Van Remoortere wrote:
> We'd have to cast the second secs() call to an integer for the math to be correct. Reusing tval.tv_sec doest that in a more readable way?
ah right, yes.
answering your first question, I found it in the man page for sys/time.h. But:
sys/time.h defers to bits/time.h which defines timeval:
30 struct timeval
31 {
32 __time_t tv_sec; /* Seconds. */
33 __suseconds_t tv_usec; /* Microseconds. */
34 };
which uses types from bits/types.h defined in bits/typesizes.h:
64 #define __TIME_T_TYPE __SYSCALL_SLONG_TYPE
65 #define __USECONDS_T_TYPE __U32_TYPE
Which should be fine to assume is long (and u32), but the type agnostic approach is almost certainly safer. Unless we're narrowing from a 64-bit double to a u32, which could be bad.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29226/#review65654
-----------------------------------------------------------
On Dec. 18, 2014, 4:20 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29226/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2014, 4:20 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See Summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp f3447055eb5a0f6abf55211f337e062ed2cc607e
> 3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 4269d3c8964dce920beb8a40ef72caf075aa0a66
>
> Diff: https://reviews.apache.org/r/29226/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 29226: Add construction and conversion from / to
timeval for Duration.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Dec. 19, 2014, 7:11 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp, line 93
> > <https://reviews.apache.org/r/29226/diff/1/?file=796956#file796956line93>
> >
> > tv_sec and tv_usec are the right types to use, just in case they're not long int on some platforms.
>
> Joris Van Remoortere wrote:
> Where do you find these types?
> I'm only familiar with them being long int (e.g. http://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html)
> I can re-write it to be type agnostic, but then we lose readability:
> ```
> struct timeval tval;
> tval.tv_sec = secs();
> tval.tv_usec = us() - (tval.tv_sec * MILLISECONDS);
> return tval;
> ```
>
> Dominic Hamon wrote:
> i think that's as readable. maybe even:
>
> struct timeval tval;
> tval.tv_sec = secs();
> tval.tv_usec = us() - (secs() * MILLISECONDS);
> return tval;
>
> given that duplicate calls to secs() are cheap and will almost certainly be optimized out.
We'd have to cast the second secs() call to an integer for the math to be correct. Reusing tval.tv_sec doest that in a more readable way?
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29226/#review65654
-----------------------------------------------------------
On Dec. 19, 2014, 12:20 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29226/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2014, 12:20 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See Summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp f3447055eb5a0f6abf55211f337e062ed2cc607e
> 3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 4269d3c8964dce920beb8a40ef72caf075aa0a66
>
> Diff: https://reviews.apache.org/r/29226/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 29226: Add construction and conversion from / to
timeval for Duration.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Dec. 19, 2014, 11:11 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp, line 93
> > <https://reviews.apache.org/r/29226/diff/1/?file=796956#file796956line93>
> >
> > tv_sec and tv_usec are the right types to use, just in case they're not long int on some platforms.
>
> Joris Van Remoortere wrote:
> Where do you find these types?
> I'm only familiar with them being long int (e.g. http://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html)
> I can re-write it to be type agnostic, but then we lose readability:
> ```
> struct timeval tval;
> tval.tv_sec = secs();
> tval.tv_usec = us() - (tval.tv_sec * MILLISECONDS);
> return tval;
> ```
i think that's as readable. maybe even:
struct timeval tval;
tval.tv_sec = secs();
tval.tv_usec = us() - (secs() * MILLISECONDS);
return tval;
given that duplicate calls to secs() are cheap and will almost certainly be optimized out.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29226/#review65654
-----------------------------------------------------------
On Dec. 18, 2014, 4:20 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29226/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2014, 4:20 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See Summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp f3447055eb5a0f6abf55211f337e062ed2cc607e
> 3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 4269d3c8964dce920beb8a40ef72caf075aa0a66
>
> Diff: https://reviews.apache.org/r/29226/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 29226: Add construction and conversion from / to
timeval for Duration.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29226/#review65654
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp
<https://reviews.apache.org/r/29226/#comment108867>
tv_sec and tv_usec are the right types to use, just in case they're not long int on some platforms.
- Dominic Hamon
On Dec. 18, 2014, 4:20 p.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29226/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2014, 4:20 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See Summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp f3447055eb5a0f6abf55211f337e062ed2cc607e
> 3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 4269d3c8964dce920beb8a40ef72caf075aa0a66
>
> Diff: https://reviews.apache.org/r/29226/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>
Re: Review Request 29226: Add construction and conversion from / to
timeval for Duration.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29226/#review66110
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp
<https://reviews.apache.org/r/29226/#comment109469>
FYI, this didn't build on OS X, so I cleaned it up.
- Benjamin Hindman
On Dec. 19, 2014, 12:20 a.m., Joris Van Remoortere wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29226/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2014, 12:20 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See Summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp f3447055eb5a0f6abf55211f337e062ed2cc607e
> 3rdparty/libprocess/3rdparty/stout/tests/duration_tests.cpp 4269d3c8964dce920beb8a40ef72caf075aa0a66
>
> Diff: https://reviews.apache.org/r/29226/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Joris Van Remoortere
>
>