+1

Using range-based for-loop whenever possible improves code readability and
makes code less prone to human error.

I did some preliminary research on Google and did not find any complaint
about its performance drawback. Here is one piece from StackOverflow for
reference:
https://stackoverflow.com/questions/10821756/is-the-ranged-based-for-loop-beneficial-to-performance

Lin

On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland <
kellen.sunderl...@gmail.com> 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 <cjolivie...@apache.org>
> 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 <
> > kellen.sunderl...@gmail.com> 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());
> > > }
> > >
> >
>

Reply via email to