mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Olivier <cjolivie...@gmail.com>
Subject Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
Date Fri, 05 Oct 2018 04:08:25 GMT
-0.5 if keeping it as a warning.


On Thu, Oct 4, 2018 at 1:23 AM kellen sunderland <
kellen.sunderland@gmail.com> wrote:

> I agree that active C++ developers should be the ones making these
> choices.  The downside to that is that many of these people are already
> pretty busy.  To make the best use possible of their time it would probably
> make sense to create a concise doc with proposed style changes and ETAs
> rather than focusing on a single change (modernized range loops).  We've
> got the infrastructure in place now to support any decisions made, and I
> think it's in the best interest of the project as it will (1) unify coding
> style to help readability and (2) make the lifes easier for reviewers which
> are a critical resource on this project.
>
> I've update my PR such that it still modernizes all existing loops, but it
> will now only issue a warning in the case that someone commits an older or
> slower version of a loop.  Of course as we've mentioned a few times this
> only applies to cases where loops can be drop-in replaces with range
> loops.  I.e. no loops indexes are actively used, etc.
>
> Would you be alright with merging this change with warnings instead of
> errors for the time being Chris?
>
> On Wed, Oct 3, 2018 at 7:20 PM Pedro Larroy <pedro.larroy.lists@gmail.com>
> wrote:
>
> > +1
> >
> > @Chris: do you have data on the performance difference? As far as I know
> > there's a "rewrite rule" like the one between lambdas and C++ functors,
> so
> > performance should be very well defined, but maybe there's something that
> > you are point out that we are missing.
> >
> > Having a consistent and concise code base is beneficial, I think what
> > Kellen is advocating is to use range loops whenever possible, not
> > prescribing its usage on every case, if you have to iterate backwards
> there
> > are other ways such as backwards iterator or others.
> >
> > On Fri, Sep 28, 2018 at 6:54 AM 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