> 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`
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.
- Ben
-----------------------------------------------------------
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
>
>
