mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kellen sunderland <kellen.sunderl...@gmail.com>
Subject Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
Date Sun, 30 Sep 2018 07:28:21 GMT
About 60 but they're all addressed In the ref PR.

On Sun, Sep 30, 2018, 6:12 AM Chris Olivier <cjolivier01@gmail.com> wrote:

> How many errors exist in the code base right now if it were to be enabled?
>
> On Sat, Sep 29, 2018 at 7:03 PM Naveen Swamy <mnnaveen@gmail.com> wrote:
>
> > Thanks Kellen & Anton, for your detailed explanation and links to
> > advantages, appreciate it.
> > changing my vote to *-0*, I suggest to show as warnings.
> >
> > On Sat, Sep 29, 2018 at 8:06 PM Anton Chernov <mechernov@gmail.com>
> wrote:
> >
> > > And if you want a more authoritative opinion on that check out what the
> > C++
> > > core guidelines are saying [1]:
> > >
> > > > ES.71: Prefer a range-for-statement to a for-statement when there is
> a
> > > choice
> > > > Reason
> > > > Readability. Error prevention. Efficiency.
> > >
> > > Best regards
> > > Anton
> > >
> > > [1]
> > >
> > >
> >
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-for-range
> > >
> > >
> > > сб, 29 сент. 2018 г. в 16:13, Anton Chernov <mechernov@gmail.com>:
> > >
> > > > +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.sunderland@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 <mnnaveen@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 <
> > cjolivier01@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.sunderland@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 <
> > > >> cjolivier01@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.sunderland@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());
> > > >> > > > > > }
> > > >> > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message