mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <a...@mesosphere.io>
Subject Re: Review Request 26229: Expose poll interval from the reaper.
Date Wed, 08 Oct 2014 10:06:51 GMT


> On Oct. 6, 2014, 10:02 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/reap.cpp, lines 124-127
> > <https://reviews.apache.org/r/26229/diff/1/?file=710088#file710088line124>
> >
> >     Why do you need a variable for this? Can't this just be a 'return' statement?
> >     
> >     If there's a reason to keep this statement separate from the return, please
avoid 'auto', we'd like to start using it very conservatively.
> 
> Alexander Rukletsov wrote:
>     Naming the result of the expression serves two purposes here: clarity about what
we calculate and return and facilitating NRWO. Since auto is replaced, I mark this issue as
fixed, please reopen if you think further discussion about `return` is needed.
> 
> Michael Park wrote:
>     My thoughts on the two purposes:
>     
>     1. I actually think a comment would do serve the purpose better in terms of being
clear on what we're calculating. `adjustedInterval` doesn't actually give me that much more
information.
>     2. Facilitating NRVO I don't think is a good reason. It's much more likely for URVO
to kick in than NRVO.
> 
> Alexander Rukletsov wrote:
>     1. We already have a top-level comment describing the approach and one more right
before interpolation code. That's why I think third comment is redundant and naming a variable
suffices.
>     2. After the closer look at the code I agree with you: NRVO won't kick in here. I'll
amend it to facilitate URVO.
> 
> Michael Park wrote:
>     1. Sorry! I missed the above comments.
>     2. Well, in this case it will. But it also doesn't matter since the only member in
`Duration` is a single `int64_t`. All I was saying is that in general URVO is more likely
to kick in than NRVO. So if there's an option to choose, we should prefer URVO (unless of
course, naming a variable helps readability).

Why would NRVO work here? We have two other paths returning rvalues.


- Alexander


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


On Oct. 8, 2014, 9:30 a.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26229/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2014, 9:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Till
Toenshoff.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Lower and upper bounds for the poll interval are refactored as static functions visible
to outer world.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/reap.hpp 9de5336 
>   3rdparty/libprocess/src/reap.cpp ac14a86 
> 
> Diff: https://reviews.apache.org/r/26229/diff/
> 
> 
> Testing
> -------
> 
> make check (Mac OS 10.9.4, Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message