> 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.
>
> Alexander Rukletsov wrote:
> Makes sense, especially `++containerizer`. To make the example less
> confusing, I change it to `.find()`.
>
> Ben Mahler wrote:
> Great, Alexander and I chatted off thread as well to go with an example
> of .find().
>
> Ben: If your example (1) is calling map::find (vector::find does not
> exist), then you're actually getting an iterator to map<K,V>::value_type.
> This means you need to do 2 operations that you didn't show above:
>
> ```
> auto containerizer = containerizers.find("docker");
>
> // This doesn't not work!
> // containerizer->launch(...);
>
> // (1) First need to check validity.
> if (containerizer != containerizers.end()) {
> // (2) Have to use get the mapped type out of the iterator with ->second
> containerizer->second->launch(...);
> }
> ```
>
> That's why I think even for (1) it's strange to call it 'containerizer'.
> If we were getting an `Option<Containerizer*>` from `hashmap::get` then that
> would be a good name.
>
> Grepping through the code I don't see any cases of your naming scheme in
> (1)? FWICT we always call the result of find() an 'iterator', or 'i'.
Both approaches have their disadvantages. This
auto containerizer = containerizers.find("docker");
containerizer->second->launch(...);
is confusing since it's not a containerizer. This
auto i = containerizers.find("docker");
i->second->launch(...);
is too general, it doesn't say what will be launched. Since we introduce `auto`
we definitely want to mention both the type and its meaning. How about
something like
auto containerizerIt = containerizers.find("docker");
containerizerIt->second->launch(...);
I will update the patch with this proposal.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25622/#review54173
-----------------------------------------------------------
On Sept. 25, 2014, 11:47 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25622/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2014, 11:47 a.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
>
>
