ok then, my vote is still -1, however, because it’s just adding needless friction for developers imho.
On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland < [email protected]> wrote: > "Range loops aren’t always the most performant way" Do you have an example > where there's a perf difference? > > "In addition, sometimes you want the index. Or maybe you want to iterate > backwards, or not start from the first, etc. Maybe you want the iterator > because you remove it from the list at the bottom of the loop.... Seems > like a rule for the sake of having a rule." > > I should have been more clear about this point. If you're using the index > in the loop, doing reverse iteration, or not iterating from start-to-end > this inspection is smart enough to realize it and will not suggest > optimizing that type of loop. The loops that would be changes are _only_ > the loops which are detected as equivalent to range-loops. Examples can be > found here: > https://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html > or you can look at what's been changed in the ref PR. I've initially set > our confidence level at 'reasonable' but we could also set to 'safe' which > would further reduce the number of loops the check would apply to. > > -Kellen > > On Fri, Sep 28, 2018 at 3:54 PM Chris Olivier <[email protected]> > wrote: > > > -1 > > > > Range loops aren’t always the most performant way. In addition, sometimes > > you want the index. Or maybe you want to iterate backwards, or not start > > from the first, etc. Maybe you want the iterator because you remove it > from > > the list at the bottom of the loop.... Seems like a rule for the sake of > > having a rule. > > > > On Fri, Sep 28, 2018 at 2:12 AM kellen sunderland < > > [email protected]> wrote: > > > > > Hello MXNet devs, > > > > > > I'd like to discuss uniformly adopting C++11 range loops in the MXNet > > > project. The benefits I see are: > > > > > > * Improved C++ readability (examples below). > > > * Consistency with other languages. The range-loops are quite similar > > to > > > loops almost all other programming languages. Given we're a project > that > > > supports many languages this language consistency could be positive for > > our > > > community. > > > * Consistency within the same project. Currently different authors > have > > > different loops styles which hurts codebase readability. > > > * Best available performance. There are often multiple ways to write > > > loops in C++ with subtle differences in performance and memory usage > > > between loop methods. Using range-loops ensures we get the best > possible > > > perf using an intuitive loop pattern. > > > * Slightly lower chance for bugs / OOB accesses when dealing with > > indexing > > > in an array for example. > > > > > > If we decide to enable this uniformly throughout the project we can > > enable > > > this policy with a simple clang-tidy configuration change. There would > > be > > > no need for reviewers to have to manually provide feedback when someone > > > uses an older C++ loops style. > > > > > > -Kellen > > > > > > Reference PR: https://github.com/apache/incubator-mxnet/pull/12356/ > > > Previous clang-tidy discussion on the list: > > > > > > > > > https://lists.apache.org/thread.html/b0ae5a9df5dfe0d9074cb2ebe432264db4fa2175b89fa43a5f6e36be@%3Cdev.mxnet.apache.org%3E > > > > > > ------------------------- > > > Examples: > > > for (auto axis_iter = param.axis.begin() ; axis_iter!= > param.axis.end(); > > > ++axis_iter) { > > > CHECK_LT(*axis_iter, static_cast<int>(ishape.ndim())); > > > stride_[reverse_index] = ishape[*axis_iter]; > > > ... > > > --> > > > for (int axis : param.axis) { > > > CHECK_LT(axis, static_cast<int>(ishape.ndim())); > > > stride_[reverse_index] = ishape[axis]; > > > ... > > > -------------------------- > > > for (size_t i = 0; i < in_array.size(); i++) { > > > auto &nd = in_array[i]; > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true, nd.dtype()); > > > } > > > --> > > > for (auto & nd : in_array) { > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true, nd.dtype()); > > > } > > > > > >
