+1
Maybe it's not necessary to enforce usage of range-based for, but I would
highly encourage to to it due to already named advantages. If code would be
introduced using the old-style there could be a comment suggesting the new
way. But why do the manual work and not leave that to the automated tool?
And since it's already automated - wouldn't it be better to keep a unified
modern style?
Just to make this a trend - C++ evolves quickly and this will not be only
upgrade that would needed to be made. And the easier such upgrades get
accepted the easier in general is to upgrade the codebase.
Soon the standard will get ranges and concepts and this will change the way
C++ applications get written significantly. It is a good habit to be open
for changes and keep up with the trends. By using the new possibilities the
language can offer you prepare yourself for further changes and are more
likely to accept them, evolving your programming style.
Take a look at a new examples on modern usages (taken from [1]):
// since C++17
for (auto&& [first,second] : mymap) {
// use first and second
}
// since C++20
for (auto& x : foo().items()) { /* .. */ } // undefined behavior if foo()
returns by value
for (T thing = foo(); auto& x : thing.items()) { /* ... */ } // OK
// since C++11
struct cow_string { /* ... */ };
// a copy-on-write string cow_string str = /* ... */;
// for(auto x : str) { /* ... */ } // may cause deep copy
for(auto x : std::as_const(str)) { /* ... */ }
Regarding performance: it's really easy to prove that generated assembly is
not changing at all. There is a really handy tool for that [2]. You can
check online the assembly for different language constructs and different
compilers.
Best regards,
Anton
[1] https://en.cppreference.com/w/cpp/language/range-for
[2] https://gcc.godbolt.org
сб, 29 сент. 2018 г. в 13:15, kellen sunderland <kellen.sunderl...@gmail.com
>:
> It's more readable because it's concise and it's consistent for many types
> you're looping over (i.e. primitive arrays, stl iterators, etc all work the
> same way). It's also useful because it's consistent with other programming
> languages, making C++ codebases much easier to read for novice and
> intermediate developers. IMO it also leads to better naming in loop bodies
> as the concise style means you're less likely to have important 1 letter
> variable names describing loop elements (e.g. no int i =0 or it ...). More
> motivation can be found in the cpp standards proposals for C++11
> http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1868.html and
> http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3853.htm.
>
>
>
> On Sat, Sep 29, 2018 at 6:38 PM Naveen Swamy <mnnav...@gmail.com> wrote:
>
> > Kellen,
> >
> > Could you please explain why you think range loops are better and how it
> > improves readability? this is a relatively new feature, many of them are
> > used to the old syntax, shouldn't we leave it for the developers to
> choose
> > the one that best suits the need and their familiarity.
> > In general I support the notion of standardizing where necessary,
> enforcing
> > rules on loops seems little bit like micro-managing how you should write
> > C++ code for MXNet.
> >
> > -1(open to change based on new information)
> >
> >
> >
> > On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier <cjolivie...@gmail.com>
> > wrote:
> >
> > > 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 <
> > > 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());
> > > > > > }
> > > > > >
> > > > >
> > > >
> > >
> >
>
