> 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.
Makes sense, especially `++containerizer`. To make the example less confusing, I change it to `.find()`. - Alexander ----------------------------------------------------------- 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 > >
