mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 33558: Add C++11 lambdas to the C++ style guide.
Date Tue, 28 Apr 2015 04:21:54 GMT


> On April 28, 2015, 2:08 a.m., Ben Mahler wrote:
> > docs/mesos-c++-style-guide.md, line 162
> > <https://reviews.apache.org/r/33558/diff/3/?file=942044#file942044line162>
> >
> >     When would we use capture by reference? Seems prone to mistakes?

> When would we use capture by reference?

We would use it when we know the lambda will be invoked within the scope. For example, if
we're calling an algorithm (e.g. `std::sort`) that takes a lambda, we know that it'll be invoked
within the scope.

> Seems prone to mistakes?

It is indeed prone to mistakes which is the motivation behind `*never* use default capture
by reference`.


> On April 28, 2015, 2:08 a.m., Ben Mahler wrote:
> > docs/mesos-c++-style-guide.md, lines 206-208
> > <https://reviews.apache.org/r/33558/diff/3/?file=942044#file942044line206>
> >
> >     What does it mean to pass a lambda to `socket.send`? Doesn't look like this
is functionality provided on the existing socket we have, and I'm not sure what this would
do.. provide a functor of the data?

As this review is about style, I myself didn't dig into the validity of this example all that
much. But perhaps we should find a more realistic example.


> On April 28, 2015, 2:08 a.m., Ben Mahler wrote:
> > docs/mesos-c++-style-guide.md, lines 237-294
> > <https://reviews.apache.org/r/33558/diff/3/?file=942044#file942044line237>
> >
> >     Hm.. this look a bit hard to read to me, you might think that the capture list
is an argument as you scan case 1, for example, and it might be hard to distinguish the lambda
from other arguments (consider if socket.send takes more arguments).
> >     
> >     Would it be better to illustrate the wrapping via a variable?
> >     
> >     ```
> >     auto lambda = [&capture1, &capture2, &capture3]
> >         (const T1& p1, const T2& p2, const T3& p3) {
> >           ...;
> >         };
> >     ```
> >     
> >     One last note, it seeems nice in many cases to keep the opening parenthesis
with the parameters rather than against the end of the capture list (e.g. as I did above).
I realize that's not clean to do when you have to wrap the parameters as well though, so maybe
not :)

+1 to illustrating the examples via a variable.
> One last note, it seeems nice in many cases to keep the opening parenthesis with the
parameters rather than against the end of the capture list (e.g. as I did above). I realize
that's not clean to do when you have to wrap the parameters as well though, so maybe not :)

I don't like that it would deviate further from how we format parameters of regular functions.
In specific, we don't format functions like this:

```cpp
SomeLongReturnType SomeLongFunctionName
    (const T1& p1, const T2& p2, const T3& p3)
{
  ...
}
```

we format it like this:

```cpp
SomeLongReturnType SomeLongFunctionName(
    const T1& p1, const T2& p2, const T3& p3)
{
  ...
}
```

By "further" above I'm referring to the fact that we put a newline between `) {` for regular
functions whereas for lambdas we don't.


- Michael


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


On April 26, 2015, 8:26 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33558/
> -----------------------------------------------------------
> 
> (Updated April 26, 2015, 8:26 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Joris Van Remoortere, Michael Park, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary. Note that there will be a follow up review which enacts this style in the
code base.
> 
> 
> Diffs
> -----
> 
>   docs/mesos-c++-style-guide.md fe98f90ad0b0f5dd38af97e85062e90cee8de99e 
> 
> Diff: https://reviews.apache.org/r/33558/diff/
> 
> 
> Testing
> -------
> 
> N/A
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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