Re: Review Request 25622: Update the Mesos Style Guide with C++11 and naming notes.

[Cody Maloney <co...@mesosphere.io>]

> 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'.
> 
> Alexander Rukletsov wrote:
>     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.
> 
> Ben Mahler wrote:
>     We would not expect to use an iterator immediately after calling find:
>     
>     ```
>     auto i = containerizers.find("docker");
>     i->second->launch(...); // Crash if (i == containerizers.end()) !!
>     ```
>     
>     We would need some kind of check, e.g.:
>     
>     ```
>     auto i = containerizers.find("docker");
>     
>     if (i == containerizers.end()) {
>       return Error("Docker not found");
>     }
>     
>     Containerizer* containerizer = i->second;
>     containerizer->launch(...);
>     ```
>     
>     In which case, it doesn't have the disadvantage you mentioned earlier IMO.
>     
>     Just as a note, in general we've avoided the need to work with iterators like this if we have our hashmap type.
>     
>     ```
>     if (!containerizers.contains("docker")) {
>       return Error("Docker not found");
>     }
>     
>     containerizers["docker"]->launch(...);
>     ```
>     
>     Or:
>     
>     ```
>     Option<Containerizer*> containerizer = containerizers.get("docker");
>     
>     if (containerizer.isNone()) {
>       return Error("Docker not found");
>     }
>     
>     containerizer.get()->launch(...);
>     
>     ```
>     
>     We don't have a Map type wrapper but it's been talked about a few times. Probably a bit much for now. ;)
> 
> Alexander Rukletsov wrote:
>     I think BenH is against saying
>     
>         auto containerizer = containerizers.get("docker");
>     
>     so this won't be an example of using auto. What would you say about my current suggestion:
>     
>         const auto& valueIt = values.find(keys.front());

https://www.youtube.com/watch?v=xnqTKD8uD64&list=UUMlGfpWw-RUdWX_JbLCukXg#t=1703 - Has good guidelines with justification for very liberal usage of auto.


- Cody


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


On Sept. 26, 2014, 12:32 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25622/
> -----------------------------------------------------------
> 
> (Updated Sept. 26, 2014, 12:32 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. 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
> 
>