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 25622: Update the Mesos Style Guide with C++11 and naming notes.
Date Thu, 25 Sep 2014 11:46:17 GMT


> On Sept. 22, 2014, 7:19 p.m., Ben Mahler wrote:
> > docs/mesos-c++-style-guide.md, lines 96-99
> > <https://reviews.apache.org/r/25622/diff/3/?file=699636#file699636line96>
> >
> >     Why would the iterator be called `containerizer`?
> >     
> >     s/containerizer/iterator/ ?
> 
> Dominic Hamon wrote:
>     -1
>     
>     naming a variable after a type is never a good idea. in this case, you're getting
a containerizer (iterator) from the container of containerizers so the name 'containerizer'
makes sense.
> 
> Ben Mahler wrote:
>     Sounds confusing.
> 
> Ben Mahler wrote:
>     If 'auto' was not used here, would we call this 'containerizer'? In a loop, this
would typically be called `iterator`, no?
>     
>     ```
>     for (auto iterator = containerizers.begin(); iterator != containerizers.end(); ++iterator)
{
>       Containerizer* containerizer = *iterator;
>     }
>     ```
>     
>     Why do something differently when auto is used?
>     
>     If the iterator was being "de-referenced" then `containerizer` makes sense:
>     
>     ```
>       Containerizer* containerizer = *(containerizers.begin());
>     ```
> 
> Alexander Rukletsov wrote:
>     I agree with Dominic: it's more important what is stored in the container and not
how we access it (iterator, reference, etc.). Actually, the example is taken from our code
base, see `src/slave/containerizer/composing.cpp:394`
> 
> Ben Mahler wrote:
>     Ok, since this example uses `containerizer` as a reference to the `Containerizer*`,
as opposed to an iterator, your points make sense. But in general I don't think this is a
pattern we'll want in our code because of the masquerading types now hidden with 'auto'.
>     
>     Iterators are not as simple as a pointer or reference. What I find unfortunate is
that we wouldn't apply the same naming scheme as soon as we change the container type, which
affects the iterator type:
>     
>     ```
>     map<string, Containerizer*> containerizers;
>     auto containerizer = containerizers.begin(); // Wouldn't do this.
>     ```
>     
>     How about a different example here with .find() as opposed to .begin()? Take a look
at cache.hpp as an example:
>     
>     ```
>       // Evict the least-recently used element from the cache.
>       void evict()
>       {
>         const typename map::iterator& i = values.find(keys.front());
>         CHECK(i != values.end());
>         values.erase(i);
>         keys.pop_front();
>       }
>     ```
>     
>     Here we definitely care about the iterator, as opposed to the value, and would name
'i' accordingly.
> 
> Alexander Rukletsov wrote:
>     I see. So you would like to be able to 1) distinguish between iterator type and element
type, and 2) be able to reason about the iterator type somehow (possible from the variable
name). I think, that makes sense.
>     
>     Using `auto` hides the actual type, and this can be both good and bad. We should
use it in places where we don't care (or shouldn't care) about the actual type. Do we care
about the container and iterator types when enumerating all elements? I tend to say no, but
you are absolutely right that iterators of different containers are used in different ways.
>     
>     Regarding naming, we can choose something general, like "element" or "item". It will
be clear from the context, what this element is about and implies neither value nor iterator
and can be used for both.
> 
> Benjamin Hindman wrote:
>     To the best of my knowledge we've (or at least I've) used two different naming schemes
for working with iterators in the code base:
>     
>     (1) When doing a 'find' we've named the variable the "thing" we're trying to actually
use, for example:
>     
>     auto containerizer = containerizers.find("docker");
>     
>     (2) When doing a 'begin' we've named the variable something that makes it clear it's
an iterator, for example:
>     
>     auto it = containerizers.begin();
>     auto iterator = containerizers.begin();
>     
>     Why? Because for (1) it's highly likely that you'll want to do things like:
>     
>     containerizer->launch(...);
>     containerizers.erase(containerizer);
>     
>     But not:
>     
>     ++containerizer;
>     
>     Which can be harder to read, especially when initially declared using 'auto'. Whereas
for (2) it's highly likely that we _will_ want to do:
>     
>     ++iterator
>     
>     Which when named 'iterator' is easy to understand what you're doing.
>     
>     That's my suggestion for suggesting a naming scheme for this guide.

Makes sense, especially `++containerizer`. To make the example less confusing, I change it
to `.find()`.


- Alexander


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


On Sept. 22, 2014, 1:10 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25622/
> -----------------------------------------------------------
> 
> (Updated Sept. 22, 2014, 1:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Dominic Hamon, and Till Toenshoff.
> 
> 
> Bugs: MESOS-1793
>     https://issues.apache.org/jira/browse/MESOS-1793
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Explicitly prohibit the use of namespace aliases. The discussion about using namespace
aliases took place in [the other review](https://reviews.apache.org/r/25434/#comment91754).
The majority agreed not to introduce them in code.
> 
> Give examples of preferable way of using auto.
> 
> 
> Diffs
> -----
> 
>   docs/mesos-c++-style-guide.md 59a39df 
> 
> Diff: https://reviews.apache.org/r/25622/diff/
> 
> 
> Testing
> -------
> 
> Documentation change, no `make check` needed.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


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