Ranged for #73
Ranged for #73
Conversation
`for (Iter i = c.begin(); i != c.end(); ++i)` is long, noisy, and the extra capabilities of iterators over a simple foreach are mostly not needed.
| } | ||
| for (auto const &s : subvol) { | ||
| if (ret) break; | ||
| ret = !s.deleted() && s.path() == name && (!getFormat() || s.created()); |
There was a problem hiding this comment.
Ordering of the previous two lines looks strange.
There was a problem hiding this comment.
well it preserves semantics of the old code (while (... && !ret), but i agree. should i swap the two lines?
There was a problem hiding this comment.
Yes, or try to use any_of with a lambda function thus eliminating the (explicit) for.
There was a problem hiding this comment.
i've tried std::any_of, it wants a pair of iterators again, and the lambda needs name captured. overall it was more verbose than the ranged-for, so i'm just swapping the lines.
|
The range based for is OK (but I didn't check the complete diff). But the use of auto I dislike since it makes it more difficult for the user to determine the type upon inspection (see http://en.wikipedia.org/wiki/C%2B%2B0x). |
|
the value type is usually given away a few lines above: here, |
|
But what about e.g. "for (auto const *it : getChildNodes(node, "devices"))"? You have to look at another file. |
| for (list<Node>::const_iterator node = nodes.begin(); node != nodes.end(); ++node) | ||
| out << " " << (*node) << endl; | ||
| for (auto const &node : nodes) | ||
| out << " " << (node) << endl; |
if it said |
|
Then take "for (auto const &i : partitionsKernelKnowns(parts))". Here auto is string and writing that would avoid searching for and looking at the definition of partitionsKernelKnowns. Also "string" is only two characters longer than "auto" so "overly verbose" seems exaggerated. |
|
i recognize that type inference may hinder understanding so i'll defer to you (i like them because the can make refactorings easier). should i revert all instances of |
| for (list<string>::const_iterator i = spares.begin(); i != spares.end(); ++i) | ||
| sumK = accumulate(sizes.begin(), sizes.end(), 0); | ||
| auto mit = min_element(sizes.begin(), sizes.end()); | ||
| smallestK = mit != sizes.end() ? *mit : 0; |
There was a problem hiding this comment.
How about unsigned long long smallestK = sizes.empty() ? 0 : *min_element(sizes.begin(), sizes.end());?
|
Please make use of |
openSUSE#73 (comment) this reverts `auto` declarators used in ranged-for statements to explicit typenames, with the exception of statements where the iterated range is: * a local variable declared with an explicit type * a member variable declared in the same class (not inherited) * result of a method defined in the same class (not inherited) * result of a free function defined in the same source file local variables =============== void op(vector<int> const &offsets) { list<string> labels; // ... for (auto const &i : offsets) // no need to repeat `int` use(i); // ... for (auto const &i : labels) // no need to repeat `string` use(i); } data members declared in the same class ======================================= struct fubar : snafu { vector<int> offsets; // ... void op() { for (auto i : offsets) use(i); } } methods declared in the same class ================================== struct fubar : snafu { vector<int> offsets(); // ... void op() { for (auto i : offsets()) use(i); } } free functions defined in the same source file ============================================== vector<int> offsets() { // ... } // ... struct fubar : snafu { void op() { for (auto i : offsets()) use(i); } } i posit that there's no loss of information in going from `for (PeMap::const_iterator it = x.begin(); ...)` and its ilk to `for (auto const &it : x)`, (PeMap is *obviously* a typedef for `std::map<string, unsigned long>`, right? j/k, it's not, and the fact that `PeMap::const_iterator` was ok while `auto` is not suggests that the stated reason for pushback against `auto` ("I dislike [auto] since it makes it more difficult for the user to determine the type upon inspection") is not the whole story. :) notice that the value of decltype(i)::value_type (it's Volume) is mentioned nowhere in bool Volume::loopInUse(const Storage* sto, const string& loopdev) { bool ret = false; Storage::ConstVolPair p = sto->volPair(hasLoopDevice); Storage::ConstVolIterator i = p.begin(); while (!ret && i != p.end()) { ret = i->loop_dev == loopdev; ++i; } return ret; } IMO, if ConstVolIterator is good enough to confer that the type of `i` is Volume, so is volPair(): // 1. Storage::ConstVolPair p = sto->volPair(hasLoopDevice); for (Storage::ConstVolIterator i = p.begin(); i != p.end() ++i) { ret = i->loop_dev == loopdev; if (ret) break; } // 2. Storage::ConstVolPair p = sto->volPair(hasLoopDevice); for (auto i = p.begin(); i != p.end() ++i) { ret = i->loop_dev == loopdev; if (ret) break; } // 3. for (auto const &i : sto->volPair(hasLoopDevice)) { ret = i.loop_dev == loopdev; if (ret) break; }
|
Closing due to different opinions of requester and reviewer. |
|
mere 2.5 years to sit it out, good job! |
for (Iter i = c.begin(); i != c.end(); ++i)is long, noisy,and the extra capabilities of iterators over a simple foreach
are mostly not needed.
make checkandmake -C testsuite-realare happy withthe changes, still this could use a combover from someone
with OCD. or better, a run of the acceptance tests.
@k0da, @kobliha