> 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()); > > Cody Maloney wrote: > > https://www.youtube.com/watch?v=xnqTKD8uD64&list=UUMlGfpWw-RUdWX_JbLCukXg#t=1703 > - Has good guidelines with justification for very liberal usage of auto.
Sutter is a famous auto-advocate. Here is his post about auto (text version of the video : ) ): http://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/ - Alexander ----------------------------------------------------------- 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 > >
