> 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.
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.
- Benjamin
-----------------------------------------------------------
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
>
>
